Re: [PATCH RFC 06/11] dyndbg: make _ddebug::flags its own field

["Gary Guo" <gary@garyguo.net>]

On Sun Oct 11, 2026 at 3:47 PM BST, Thomas Weißschuh wrote:
> On 2026-10-11 14:23:21+0100, Gary Guo wrote:
>> On Sun Oct 11, 2026 at 1:52 PM BST, Thomas Weißschuh wrote:
>> > Keeping the flags as part of the bitfield prevents the usage of
>> > READ_ONCE()/WRITE_ONCE() with it. It also makes access from Rust
>> > awkward.
>> >
>> > Make it a dedicated field.
>> >
>> > The new field will stay in the exact same location in the parent
>> > structure.
>> 
>> Incorrect.
>> 
>> >
>> > No functional change.
>> >
>> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> > ---
>> >  include/linux/dynamic_debug.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
>> > index 0b34343df594..b1ff7c0e9a5f 100644
>> > --- a/include/linux/dynamic_debug.h
>> > +++ b/include/linux/dynamic_debug.h
>> > @@ -51,7 +51,7 @@ struct _ddebug {
>> >  #else
>> >  #define _DPRINTK_FLAGS_DEFAULT 0
>> >  #endif
>> > -	u32 flags:8;
>> > +	u8 flags;
>> 
>> This will change the layout of the struct, because it'll no longer be merged
>> with the previous bitfields.
>
> It seems to work for me:
>
> struct _ddebug {
>         const char  *              modname;              /*     0     8 */
>         const char  *              function;             /*     8     8 */
>         const char  *              filename;             /*    16     8 */
>         const char  *              format;               /*    24     8 */
>         u32                        lineno:18;            /*    32: 0  4 */
>         u32                        class_id:6;           /*    32:18  4 */
>
>         /* Bitfield combined with next fields */
>
>         u8                         flags;                /*    35     1 */
>
>         /* XXX 4 bytes hole, try to pack */
>
>         union {
>                 struct static_key_true dd_key_true __attribute__((__aligned__(8))); /*    40    16 */
>                 struct static_key_false dd_key_false __attribute__((__aligned__(8))); /*    40    16 */
>         } key __attribute__((__aligned__(8)));           /*    40    16 */
>
>         /* size: 56, cachelines: 1, members: 8 */
>         /* sum members: 49, holes: 1, sum holes: 4 */
>         /* sum bitfield members: 24 bits (3 bytes) */
>         /* forced alignments: 1, forced holes: 1, sum forced holes: 4 */
>         /* last cacheline: 56 bytes */
> } __attribute__((__aligned__(8)));
>
> (x86_64, GCC 16.2.1)

I stand corrected. I thought the type change would force allocation unit to not
be shared; turns out that this is only behaviour of some compiler (e.g. MSVC)
where in GCC this is not the case. C standard says it's an
implementation-defined behaviour, and I guess that's why people don't like using
bit fields :)

Given kernel assumes GCC (or compatible behaviour) I suppose this is alright
and you're right that the layout is unchanged in this case. In that case I think
using what you have is fine, but this should still be merged with the following
patch that adds READ_ONCE/WRITE_ONCE.

Best,
Gary