Hi all,
I'm working on enabling -Wflex-array-member-not-at-end in mainline, and
I ran into thousands (yes, 14722 to be precise) of these warnings caused
by an instance of `struct cgroup` in the middle of `struct cgroup_root`.
See below:
620 struct cgroup_root {
...
633 /*
634 * The root cgroup. The containing cgroup_root will be destroyed on its
635 * release. cgrp->ancestors[0] will be used overflowing into the
636 * following field. cgrp_ancestor_storage must immediately follow.
637 */
638 struct cgroup cgrp;
639
640 /* must follow cgrp for cgrp->ancestors[0], see above */
641 struct cgroup *cgrp_ancestor_storage;
...
};
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.
However when I take a look at the pahole output, I see that these two members
are actually misaligned by 56 bytes. See below:
struct cgroup_root {
...
/* --- cacheline 1 boundary (64 bytes) --- */
struct cgroup cgrp __attribute__((__aligned__(64))); /* 64 2112 */
/* XXX last struct has 56 bytes of padding */
/* --- cacheline 34 boundary (2176 bytes) --- */
struct cgroup * cgrp_ancestor_storage; /* 2176 8 */
...
/* size: 6400, cachelines: 100, members: 11 */
/* sum members: 6336, holes: 1, sum holes: 16 */
/* padding: 48 */
/* paddings: 1, sum paddings: 56 */
/* forced alignments: 1, forced holes: 1, sum forced holes: 16 */
} __attribute__((__aligned__(64)));
This is due to the fact that struct cgroup have some tailing padding after
flexible-array member `ancestors` due to alignment to 64 bytes, see below:
struct cgroup {
...
struct cgroup * ancestors[]; /* 2056 0 */
/* size: 2112, cachelines: 33, members: 43 */
/* sum members: 2000, holes: 3, sum holes: 56 */
/* padding: 56 */
/* paddings: 2, sum paddings: 8 */
/* forced alignments: 1 */
} __attribute__((__aligned__(64)));
The offset for `ancestors` is at 2056, but sizeof(struct group) == 2112 due
to the 56 bytes of tailing padding. This looks buggy. (thinkingface)
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:
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 539c64eeef38..901a46f70a02 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -630,16 +630,6 @@ struct cgroup_root {
struct list_head root_list;
struct rcu_head rcu; /* Must be near the top */
- /*
- * The root cgroup. The containing cgroup_root will be destroyed on its
- * release. cgrp->ancestors[0] will be used overflowing into the
- * following field. cgrp_ancestor_storage must immediately follow.
- */
- struct cgroup cgrp;
-
- /* must follow cgrp for cgrp->ancestors[0], see above */
- struct cgroup *cgrp_ancestor_storage;
-
/* Number of cgroups in the hierarchy, used only for /proc/cgroups */
atomic_t nr_cgrps;
@@ -651,6 +641,18 @@ struct cgroup_root {
/* The name for this hierarchy - may be empty */
char name[MAX_CGROUP_ROOT_NAMELEN];
+
+ /*
+ * The root cgroup. The containing cgroup_root will be destroyed on its
+ * release. cgrp->ancestors[0] will be used overflowing into the
+ * following field. cgrp_ancestor_storage must immediately follow.
+ *
+ * Must be last --ends in a flexible-array members.
+ */
+ TRAILING_OVERLAP(struct cgroup, cgrp, ancestors,
+ /* must follow cgrp for cgrp->ancestors[0], see above */
+ struct cgroup *cgrp_ancestor_storage;
+ );
};
However, this causes the size of struct cgroup_root to increase from 6400
bytes to 16384 bytes due to struct cgroup to be aligned to page size 4096
bytes. See below:
struct cgroup_root {
struct kernfs_root * kf_root; /* 0 8 */
unsigned int subsys_mask; /* 8 4 */
int hierarchy_id; /* 12 4 */
struct list_head root_list; /* 16 16 */
struct callback_head rcu __attribute__((__aligned__(8))); /* 32 16 */
atomic_t nr_cgrps; /* 48 4 */
unsigned int flags; /* 52 4 */
char name[64]; /* 56 64 */
/* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */
char release_agent_path[4096]; /* 120 4096 */
/* XXX 3976 bytes hole, try to pack */
/* --- cacheline 128 boundary (8192 bytes) --- */
union {
struct cgroup cgrp __attribute__((__aligned__(4096))); /* 8192 8192 */
struct {
unsigned char __offset_to_ancestors[5784]; /* 8192 5784 */
/* --- cacheline 218 boundary (13952 bytes) was 24 bytes ago --- */
struct cgroup * cgrp_ancestor_storage; /* 13976 8 */
}; /* 8192 5792 */
} __attribute__((__aligned__(4096))); /* 8192 8192 */
/* size: 16384, cachelines: 256, members: 10 */
/* sum members: 12408, holes: 1, sum holes: 3976 */
/* forced alignments: 2, forced holes: 1, sum forced holes: 3976 */
} __attribute__((__aligned__(4096)));
I've tried with the struct_group_tagged()/container_of() technique:
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/wfamnae-next20250723&id=03da6b0772af1a62778400f26fe57796fe1ebf27
but cgroup_root grows up to 20K in this case.
So, I guess my question here is... what do you think?... (thinkingface)
Thanks!
-GustavoOn 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.
Why do you move struct cgroup at the end of struct cgroup_root?
(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?)
Thanks,
Michal
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;
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
On Mon, Sep 01, 2025 at 05:21:22PM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > 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. Oh, so TRAILING_OVERLAP() won't work like that? (I thought that it'd hide the FAM from the end of the union and thus it could embedded when wrapped like this. On second thought, I realize that's exclusive with the static validations.) > 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. :) I'd prefer if cgroup_create could still work with one allocation only both for struct cgroup and its ancestors array. (Cgroup allocation happens many times in a day.) The increase in struct cgroup_root size is IMO not that problematic. (There are typically at most CGROUP_SUBSYS_COUNT roots with gradual trend to only the single cgrp_dfl_root.) Note that it'd be good to keep it enclosed within struct cgroup_root (cgroup1_root_to_use could use struct_size()), however, the cgrp_dfl_root would still need the storage somewhere. HTH, Michal
On 9/1/25 19:58, Michal Koutný wrote:
> On Mon, Sep 01, 2025 at 05:21:22PM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>> 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.
>
> Oh, so TRAILING_OVERLAP() won't work like that?
> (I thought that it'd hide the FAM from the end of the union and thus it
> could embedded when wrapped like this. On second thought, I realize
> that's exclusive with the static validations.)
>
>> 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. :)
>
> I'd prefer if cgroup_create could still work with one allocation only
> both for struct cgroup and its ancestors array. (Cgroup allocation
> happens many times in a day.)
>
> The increase in struct cgroup_root size is IMO not that problematic.
> (There are typically at most CGROUP_SUBSYS_COUNT roots with gradual
> trend to only the single cgrp_dfl_root.)
>
> Note that it'd be good to keep it enclosed within struct cgroup_root
> (cgroup1_root_to_use could use struct_size()), however, the
> cgrp_dfl_root would still need the storage somewhere.
If the increase in size is not a problem, then something like this
works fine (unless there is a problem with moving those two members
at the end of cgroup_root?):
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 539c64eeef38..bd28d639a78a 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -630,16 +630,6 @@ struct cgroup_root {
struct list_head root_list;
struct rcu_head rcu; /* Must be near the top */
- /*
- * The root cgroup. The containing cgroup_root will be destroyed on its
- * release. cgrp->ancestors[0] will be used overflowing into the
- * following field. cgrp_ancestor_storage must immediately follow.
- */
- struct cgroup cgrp;
-
- /* must follow cgrp for cgrp->ancestors[0], see above */
- struct cgroup *cgrp_ancestor_storage;
-
/* Number of cgroups in the hierarchy, used only for /proc/cgroups */
atomic_t nr_cgrps;
@@ -651,7 +641,21 @@ struct cgroup_root {
/* The name for this hierarchy - may be empty */
char name[MAX_CGROUP_ROOT_NAMELEN];
+
+ /*
+ * The root cgroup. The containing cgroup_root will be destroyed on its
+ * release. cgrp->ancestors[0] will be used overflowing into the
+ * following field. cgrp_ancestor_storage must immediately follow.
+ *
+ * Must be last --ends in a flexible-array member.
+ */
+ TRAILING_OVERLAP(struct cgroup, cgrp, ancestors,
+ /* must follow cgrp for cgrp->ancestors[0], see above */
+ struct cgroup *cgrp_ancestor_storage;
+ );
};
+static_assert(offsetof(struct cgroup_root, cgrp.ancestors) ==
+ offsetof(struct cgroup_root, cgrp_ancestor_storage));
The assert above checks that no misalignment is inadvertently
introduced between FAM and cgrp_ancestor_storage.
Thanks
-Gustavo
On Tue, Sep 02, 2025 at 09:56:34AM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> If the increase in size is not a problem, then something like this
> works fine (unless there is a problem with moving those two members
> at the end of cgroup_root?):
Please don't forget to tackle cgroup_root allocators. IIUC, this move
towards the end shifts the burden to them.
There's only the rcu_head we care about.
(You seem to be well versed with flex arrays, I was wondering if
something like this could be rearranged to make it work (assuming the
union is at the end of its containers):
union {
struct cgroup *ancestors[];
struct {
struct cgroup *_root_ancestor;
struct cgroup *_low_ancestors[];
};
};
)
Thanks,
Michal
On 9/2/25 13:17, Michal Koutný wrote:
> On Tue, Sep 02, 2025 at 09:56:34AM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>> If the increase in size is not a problem, then something like this
>> works fine (unless there is a problem with moving those two members
>> at the end of cgroup_root?):
>
> Please don't forget to tackle cgroup_root allocators. IIUC, this move
> towards the end shifts the burden to them.
I don't see how placing the TRAILING_OVERLAP() change at the end
of cgroup_root would cause problems in cgroup_create(). I see
this allocation for `struct cgroup *cgrp`:
cgrp = kzalloc(struct_size(cgrp, ancestors, (level + 1)), GFP_KERNEL);
but I don't see why struct cgroup cgrp; and struct cgroup *cgrp_ancestor_storage;
cannot be placed at the end (as long as they're enclosed in TRAILING_OVERLAP()
of course) of cgroup_root. In the end, it seems you're only interested in
having cgrp->ancestors[0] overlap `cgrp_ancestor_storage` so that the latter
points to the start of the FAM in struct cgroup.
>
> There's only the rcu_head we care about.
Based on this commit a7fb0423c201 ("cgroup: Move rcu_head up near the
top of cgroup_root"), as long as rcu_head is not after struct cgroup,
all's fine.
However, this tells me that people were aware of the possibility of
`cgrp.ancestors[]` growing even beyond `cgrp_ancestor_storage`, which
is yet another reason not to have that flex array in the middle of
cgroup_root.
>
> (You seem to be well versed with flex arrays, I was wondering if
> something like this could be rearranged to make it work (assuming the
> union is at the end of its containers):
>
> union {
> struct cgroup *ancestors[];
> struct {
> struct cgroup *_root_ancestor;
> struct cgroup *_low_ancestors[];
> };
> };
> )
Yep, that works (as long as it's always at the very end of any container
or ends last in any nested structs, for instance in struct cgroup_root,
it must also be at the end) for GCC-15+, but for older versions of GCC we
have to use the DECLARE_FLEX_ARRAY() helper as below:
union {
/* All ancestors including self */
DECLARE_FLEX_ARRAY(struct cgroup *, ancestors);
struct {
struct cgroup *_root_ancestor;
struct cgroup *_low_ancestors[];
};
};
Thanks
-Gustavo
On Tue, 2 Sep 2025 14:37:40 +0200
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> On 9/2/25 13:17, Michal Koutný wrote:
> > On Tue, Sep 02, 2025 at 09:56:34AM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
...
> >
> > (You seem to be well versed with flex arrays, I was wondering if
> > something like this could be rearranged to make it work (assuming the
> > union is at the end of its containers):
> >
> > union {
> > struct cgroup *ancestors[];
> > struct {
> > struct cgroup *_root_ancestor;
> > struct cgroup *_low_ancestors[];
> > };
> > };
> > )
>
> Yep, that works (as long as it's always at the very end of any container
> or ends last in any nested structs, for instance in struct cgroup_root,
> it must also be at the end) for GCC-15+, but for older versions of GCC we
> have to use the DECLARE_FLEX_ARRAY() helper as below:
Could the warning be disabled for 'older versions of gcc'?
A build time warning doesn't need to happen for all builds.
David
>
> union {
> /* All ancestors including self */
> DECLARE_FLEX_ARRAY(struct cgroup *, ancestors);
> struct {
> struct cgroup *_root_ancestor;
> struct cgroup *_low_ancestors[];
> };
> };
>
> Thanks
> -Gustavo
>
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
On Mon, Sep 01, 2025 at 05:44:38PM +0200, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> 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.
My intention with the attribute was to prevent a gap (padding) occurring
between
unsigned char __offset_to_##FAM
and
MEMBERS
which would make the address of the first member to mismatch the address
of FAM (the example in struct cgroup_root notwithstanding).
(But perhaps it's guaranteed that first member's offset in the struct is
always equal to offsetof(TYPE, FAM).)
Michal
On 2025/8/30 21:30, Gustavo A. R. Silva wrote:
> Hi all,
>
> I'm working on enabling -Wflex-array-member-not-at-end in mainline, and
> I ran into thousands (yes, 14722 to be precise) of these warnings caused
> by an instance of `struct cgroup` in the middle of `struct cgroup_root`.
> See below:
>
> 620 struct cgroup_root {
> ...
> 633 /*
> 634 * The root cgroup. The containing cgroup_root will be destroyed on its
> 635 * release. cgrp->ancestors[0] will be used overflowing into the
> 636 * following field. cgrp_ancestor_storage must immediately follow.
> 637 */
> 638 struct cgroup cgrp;
> 639
> 640 /* must follow cgrp for cgrp->ancestors[0], see above */
> 641 struct cgroup *cgrp_ancestor_storage;
> ...
> };
>
> 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.
>
> However when I take a look at the pahole output, I see that these two members
> are actually misaligned by 56 bytes. See below:
>
> struct cgroup_root {
> ...
>
> /* --- cacheline 1 boundary (64 bytes) --- */
> struct cgroup cgrp __attribute__((__aligned__(64))); /* 64 2112 */
>
> /* XXX last struct has 56 bytes of padding */
>
> /* --- cacheline 34 boundary (2176 bytes) --- */
> struct cgroup * cgrp_ancestor_storage; /* 2176 8 */
>
> ...
>
> /* size: 6400, cachelines: 100, members: 11 */
> /* sum members: 6336, holes: 1, sum holes: 16 */
> /* padding: 48 */
> /* paddings: 1, sum paddings: 56 */
> /* forced alignments: 1, forced holes: 1, sum forced holes: 16 */
> } __attribute__((__aligned__(64)));
>
> This is due to the fact that struct cgroup have some tailing padding after
> flexible-array member `ancestors` due to alignment to 64 bytes, see below:
>
> struct cgroup {
> ...
>
> struct cgroup * ancestors[]; /* 2056 0 */
>
Instead of using a flexible array member, could we convert this to a pointer and handle the memory
allocation explicitly?
--
Best regards,
Ridong
On 9/1/25 03:29, Chen Ridong wrote:
>
>
> On 2025/8/30 21:30, Gustavo A. R. Silva wrote:
>> Hi all,
>>
>> I'm working on enabling -Wflex-array-member-not-at-end in mainline, and
>> I ran into thousands (yes, 14722 to be precise) of these warnings caused
>> by an instance of `struct cgroup` in the middle of `struct cgroup_root`.
>> See below:
>>
>> 620 struct cgroup_root {
>> ...
>> 633 /*
>> 634 * The root cgroup. The containing cgroup_root will be destroyed on its
>> 635 * release. cgrp->ancestors[0] will be used overflowing into the
>> 636 * following field. cgrp_ancestor_storage must immediately follow.
>> 637 */
>> 638 struct cgroup cgrp;
>> 639
>> 640 /* must follow cgrp for cgrp->ancestors[0], see above */
>> 641 struct cgroup *cgrp_ancestor_storage;
>> ...
>> };
>>
>> 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.
>>
>> However when I take a look at the pahole output, I see that these two members
>> are actually misaligned by 56 bytes. See below:
>>
>> struct cgroup_root {
>> ...
>>
>> /* --- cacheline 1 boundary (64 bytes) --- */
>> struct cgroup cgrp __attribute__((__aligned__(64))); /* 64 2112 */
>>
>> /* XXX last struct has 56 bytes of padding */
>>
>> /* --- cacheline 34 boundary (2176 bytes) --- */
>> struct cgroup * cgrp_ancestor_storage; /* 2176 8 */
>>
>> ...
>>
>> /* size: 6400, cachelines: 100, members: 11 */
>> /* sum members: 6336, holes: 1, sum holes: 16 */
>> /* padding: 48 */
>> /* paddings: 1, sum paddings: 56 */
>> /* forced alignments: 1, forced holes: 1, sum forced holes: 16 */
>> } __attribute__((__aligned__(64)));
>>
>> This is due to the fact that struct cgroup have some tailing padding after
>> flexible-array member `ancestors` due to alignment to 64 bytes, see below:
>>
>> struct cgroup {
>> ...
>>
>> struct cgroup * ancestors[]; /* 2056 0 */
>>
>
> Instead of using a flexible array member, could we convert this to a pointer and handle the memory
> allocation explicitly?
>
Yep, that's always an option. However, I also wanted to see what people
think about the current misalignment between cgrp->ancestors[0] and
cgrp_ancestor_storage I describe above.
And if the heap allocation is an acceptable solution in this case, I'm
happy to go that route.
Thanks
-Gustavo
© 2016 - 2025 Red Hat, Inc.