Re: [RFC] cgroup: Avoid thousands of -Wflex-array-member-not-at-end warnings

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 9/1/25 17:21, Gustavo A. R. Silva wrote:
> 
> 
> On 9/1/25 15:37, Michal Koutný wrote:
>> On Sat, Aug 30, 2025 at 03:30:11PM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>>> Based on the comments above, it seems that the original code was expecting
>>> cgrp->ancestors[0] and cgrp_ancestor_storage to share the same addres in
>>> memory.
>>
>> Fortunately, it doesn't matter what the address of cgrp_ancestor_storage
>> is. The important effect is that cgroup_root::cgrp is followed by
>> sufficient space to store a pointer (accessed via cgroup::ancestors[0]).
>>
>>> However when I take a look at the pahole output, I see that these two members
>>> are actually misaligned by 56 bytes. See below:
>>
>> So the root cgroup's ancestry may be saved inside the padding instead of
>> the dedicated storage. I don't think it causes immediate issues but it'd
>> be better not to write to these bytes. (Note that the layout depends on
>> kernel config.) Thanks for the report Gustavo!
>>
>>
>>> So, one solution for this is to use the TRAILING_OVERLAP() helper and
>>> move these members at the end of `struct cgroup_root`. With this the
>>> misalignment disappears (together with the 14722 warnings :) ), and now
>>> both cgrp->ancestors[0] and cgrp_ancestor_storage share the same address
>>> in memory. See below:
>>
>> I didn't know TRAILING_OVERLAP() but it sounds like the tool for such
>> situations.
> 
> I recently introduced it. :)
> 
>> Why do you move struct cgroup at the end of struct cgroup_root?
> 
> Because struct cgroup ends in a flexible-array member `ancestors`.
> This triggers the -Wflex-array-member-not-at-end warns about. So,
> while `ancestors` is indeed a flexible array, any instance of
> cgroup embedded in another struct should be placed at the end.
> 
> However, if we change it to something like this (and of course
> updating any related code, accordingly):
> 
> -       struct cgroup *ancestors[];
> +       struct cgroup **ancestors;
> 
> Then the flex in the middle issue goes away, and we can have
> struct cgroup embedded in another struct anywhere.
> 
> The question is if this would be an acceptable solution?
> 
> I'd probably prefer this to remain a flexible-array member,
> but I'd like to hear people's opinions and feedback. :)
> 
>>
>> (Actually, as I look at the macro's implementation, it should be
>> --- a/include/linux/stddef.h
>> +++ b/include/linux/stddef.h
>> @@ -110,7 +110,7 @@ enum {
>>                  struct {                                                        \
>>                          unsigned char __offset_to_##FAM[offsetof(TYPE, FAM)];   \
>>                          MEMBERS                                                 \
>> -               };                                                              \
>> +               } __packed;                                                     \
>>          }
>>
>>   #endif
>> in order to avoid similar issues, no?)
> 
> The way to do it is like this:
> 
> +       TRAILING_OVERLAP(struct cgroup, cgrp, ancestors,
> +               /* must follow cgrp for cgrp->ancestors[0], see above */
> +               struct cgroup *cgrp_ancestor_storage;
> +       ) __packed;

Oh, a correction about this. Actually, if we need to use __packed, we would
have to pass it as an argument to TRAILING_OVERLAP(), like this:

-#define TRAILING_OVERLAP(TYPE, NAME, FAM, MEMBERS)                             \
+#define TRAILING_OVERLAP(TYPE, NAME, FAM, MEMBERS, ATTRS)                              \
         union {                                                                 \
                 TYPE NAME;                                                      \
                 struct {                                                        \
                         unsigned char __offset_to_##FAM[offsetof(TYPE, FAM)];   \
                         MEMBERS                                                 \
-               };                                                              \
+               } ATTRS;                                                                \
         }

However, in this case MEMBERS is only cgrp_ancestor_storage, and it's correctly
aligned to __offset_to_##FAM[offsetof(TYPE, FAM)]; inside the helper. So, we
don't really need to pack that internal struct.

Thanks
-Gustavo

> 
> 
> and this get us to the following:
> 
> struct cgroup_root {
> ...
>          /* --- cacheline 65 boundary (4160 bytes) was 56 bytes ago --- */
>          union {
>                  struct cgroup      cgrp __attribute__((__aligned__(1))); /*  4216  8192 */
>                  struct {
>                          unsigned char __offset_to_ancestors[5784]; /*  4216  5784 */
>                          /* --- cacheline 156 boundary (9984 bytes) was 16 bytes ago --- */
>                          struct cgroup * cgrp_ancestor_storage; /* 10000     8 */
>                  };                                       /*  4216  5792 */
>          } __attribute__((__aligned__(1)));               /*  4216  8192 */
> 
>          /* size: 12408, cachelines: 194, members: 10 */
>          /* forced alignments: 2 */
>          /* last cacheline: 56 bytes */
> } __attribute__((__aligned__(8)));
> 
> Notice the change in alignment in struct cgroup_root from
> 4096 (page alignment) to 8 bytes alignment. In any case,
> the size (still) increases from 6400 to 12408 bytes.
> 
> Thanks
> -Gustavo