| Message ID | 20240203003414.1067730-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [mm-hotfixes-unstable] mm: memcg: fix struct memcg_vmstats_percpu size and alignment | expand |
On Fri, Feb 2, 2024 at 4:34 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > Commit da10d7e140196 ("mm: memcg: optimize parent iteration in > memcg_rstat_updated()") added two additional pointers to the end of > struct memcg_vmstats_percpu with CACHELINE_PADDING to put them in a > separate cacheline. This caused the struct size to increase from 1200 to > 1280 on my config (80 extra bytes instead of 16). > > Upon revisiting, the relevant struct members do not need to be on a > separate cacheline, they just need to fit in a single one. This is a > percpu struct, so there shouldn't be any contention on that cacheline > anyway. Move the members to the beginning of the struct and cachealign > the first member. Add a comment about the members that need to fit > together in a cacheline. > > The struct size is now 1216 on my config with this change. > > Fixes: da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") > Reported-by: Greg Thelen <gthelen@google.com> > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > mm/memcontrol.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index d9ca0fdbe4ab0..09f09f37e397e 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -621,6 +621,15 @@ static inline int memcg_events_index(enum vm_event_item idx) > } > > struct memcg_vmstats_percpu { > + /* Stats updates since the last flush */ > + unsigned int stats_updates ____cacheline_aligned; Why do you need ____cacheline_aligned here? From what I understand for the previous patch you want stats_updates, parent and vmstats on the same cacheline, right? I would say just remove the CACHELINE_PADDING() from the previous patch and we are good. In the followup I plan to add usage of __cacheline_group_begin() and __cacheline_group_end() usage in memcg code. If you want, take a stab at it.
On Fri, Feb 2, 2024 at 8:13 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Fri, Feb 2, 2024 at 4:34 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > Commit da10d7e140196 ("mm: memcg: optimize parent iteration in > > memcg_rstat_updated()") added two additional pointers to the end of > > struct memcg_vmstats_percpu with CACHELINE_PADDING to put them in a > > separate cacheline. This caused the struct size to increase from 1200 to > > 1280 on my config (80 extra bytes instead of 16). > > > > Upon revisiting, the relevant struct members do not need to be on a > > separate cacheline, they just need to fit in a single one. This is a > > percpu struct, so there shouldn't be any contention on that cacheline > > anyway. Move the members to the beginning of the struct and cachealign > > the first member. Add a comment about the members that need to fit > > together in a cacheline. > > > > The struct size is now 1216 on my config with this change. > > > > Fixes: da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") > > Reported-by: Greg Thelen <gthelen@google.com> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > mm/memcontrol.c | 19 +++++++++---------- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index d9ca0fdbe4ab0..09f09f37e397e 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -621,6 +621,15 @@ static inline int memcg_events_index(enum vm_event_item idx) > > } > > > > struct memcg_vmstats_percpu { > > + /* Stats updates since the last flush */ > > + unsigned int stats_updates ____cacheline_aligned; > > Why do you need ____cacheline_aligned here? From what I understand for > the previous patch you want stats_updates, parent and vmstats on the > same cacheline, right? Yes. I am trying to ensure that stats_updates sits at the beginning of a cacheline to ensure they all fit in one cacheline. Is this implicitly guaranteed somehow? > > I would say just remove the CACHELINE_PADDING() from the previous > patch and we are good. IIUC, without CACHELINE_PADDING(), they may end up on different cache lines, depending on the size of the arrays before them in the struct (which depends on several configs). Am I misunderstanding? > > In the followup I plan to add usage of __cacheline_group_begin() and > __cacheline_group_end() usage in memcg code. If you want, take a stab > at it. For now, I am just looking for something simple to fix the struct size proliferation for v6.8, but this would be interesting to see. I wonder how __cacheline_group_end() works since the end is decided already by __cacheline_group_begin() and the cacheline size.
On Fri, Feb 2, 2024 at 8:23 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Fri, Feb 2, 2024 at 8:13 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Fri, Feb 2, 2024 at 4:34 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > > > Commit da10d7e140196 ("mm: memcg: optimize parent iteration in > > > memcg_rstat_updated()") added two additional pointers to the end of > > > struct memcg_vmstats_percpu with CACHELINE_PADDING to put them in a > > > separate cacheline. This caused the struct size to increase from 1200 to > > > 1280 on my config (80 extra bytes instead of 16). > > > > > > Upon revisiting, the relevant struct members do not need to be on a > > > separate cacheline, they just need to fit in a single one. This is a > > > percpu struct, so there shouldn't be any contention on that cacheline > > > anyway. Move the members to the beginning of the struct and cachealign > > > the first member. Add a comment about the members that need to fit > > > together in a cacheline. > > > > > > The struct size is now 1216 on my config with this change. > > > > > > Fixes: da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") > > > Reported-by: Greg Thelen <gthelen@google.com> > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > > --- > > > mm/memcontrol.c | 19 +++++++++---------- > > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index d9ca0fdbe4ab0..09f09f37e397e 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -621,6 +621,15 @@ static inline int memcg_events_index(enum vm_event_item idx) > > > } > > > > > > struct memcg_vmstats_percpu { > > > + /* Stats updates since the last flush */ > > > + unsigned int stats_updates ____cacheline_aligned; > > > > Why do you need ____cacheline_aligned here? From what I understand for > > the previous patch you want stats_updates, parent and vmstats on the > > same cacheline, right? > > Yes. I am trying to ensure that stats_updates sits at the beginning of > a cacheline to ensure they all fit in one cacheline. Is this > implicitly guaranteed somehow? > > > > > I would say just remove the CACHELINE_PADDING() from the previous > > patch and we are good. > > IIUC, without CACHELINE_PADDING(), they may end up on different cache > lines, depending on the size of the arrays before them in the struct > (which depends on several configs). Am I misunderstanding? > Yeah keeping them at the start will be better. Move ____cacheline_aligned to the end of the struct definition like: struct memcg_vmstats_percpu { ... } ____cacheline_aligned; > > > > In the followup I plan to add usage of __cacheline_group_begin() and > > __cacheline_group_end() usage in memcg code. If you want, take a stab > > at it. > > For now, I am just looking for something simple to fix the struct size > proliferation for v6.8, but this would be interesting to see. I wonder > how __cacheline_group_end() works since the end is decided already by > __cacheline_group_begin() and the cacheline size.
On Fri, Feb 2, 2024 at 8:34 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Fri, Feb 2, 2024 at 8:23 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Fri, Feb 2, 2024 at 8:13 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Fri, Feb 2, 2024 at 4:34 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > > > > > Commit da10d7e140196 ("mm: memcg: optimize parent iteration in > > > > memcg_rstat_updated()") added two additional pointers to the end of > > > > struct memcg_vmstats_percpu with CACHELINE_PADDING to put them in a > > > > separate cacheline. This caused the struct size to increase from 1200 to > > > > 1280 on my config (80 extra bytes instead of 16). > > > > > > > > Upon revisiting, the relevant struct members do not need to be on a > > > > separate cacheline, they just need to fit in a single one. This is a > > > > percpu struct, so there shouldn't be any contention on that cacheline > > > > anyway. Move the members to the beginning of the struct and cachealign > > > > the first member. Add a comment about the members that need to fit > > > > together in a cacheline. > > > > > > > > The struct size is now 1216 on my config with this change. > > > > > > > > Fixes: da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") > > > > Reported-by: Greg Thelen <gthelen@google.com> > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > > > --- > > > > mm/memcontrol.c | 19 +++++++++---------- > > > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > index d9ca0fdbe4ab0..09f09f37e397e 100644 > > > > --- a/mm/memcontrol.c > > > > +++ b/mm/memcontrol.c > > > > @@ -621,6 +621,15 @@ static inline int memcg_events_index(enum vm_event_item idx) > > > > } > > > > > > > > struct memcg_vmstats_percpu { > > > > + /* Stats updates since the last flush */ > > > > + unsigned int stats_updates ____cacheline_aligned; > > > > > > Why do you need ____cacheline_aligned here? From what I understand for > > > the previous patch you want stats_updates, parent and vmstats on the > > > same cacheline, right? > > > > Yes. I am trying to ensure that stats_updates sits at the beginning of > > a cacheline to ensure they all fit in one cacheline. Is this > > implicitly guaranteed somehow? > > > > > > > > I would say just remove the CACHELINE_PADDING() from the previous > > > patch and we are good. > > > > IIUC, without CACHELINE_PADDING(), they may end up on different cache > > lines, depending on the size of the arrays before them in the struct > > (which depends on several configs). Am I misunderstanding? > > > > Yeah keeping them at the start will be better. Move > ____cacheline_aligned to the end of the struct definition like: > > struct memcg_vmstats_percpu { > ... > } ____cacheline_aligned; Will send a v2 shortly, thanks. I honestly didn't know what the difference was, but both gave me the same results. Is using ____cacheline_aligned with the first member the same as using it at the end of the definition?
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d9ca0fdbe4ab0..09f09f37e397e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -621,6 +621,15 @@ static inline int memcg_events_index(enum vm_event_item idx) } struct memcg_vmstats_percpu { + /* Stats updates since the last flush */ + unsigned int stats_updates ____cacheline_aligned; + + /* Cached pointers for fast iteration in memcg_rstat_updated() */ + struct memcg_vmstats_percpu *parent; + struct memcg_vmstats *vmstats; + + /* The above should fit a single cacheline for memcg_rstat_updated() */ + /* Local (CPU and cgroup) page state & events */ long state[MEMCG_NR_STAT]; unsigned long events[NR_MEMCG_EVENTS]; @@ -632,16 +641,6 @@ struct memcg_vmstats_percpu { /* Cgroup1: threshold notifications & softlimit tree updates */ unsigned long nr_page_events; unsigned long targets[MEM_CGROUP_NTARGETS]; - - /* Fit members below in a single cacheline for memcg_rstat_updated() */ - CACHELINE_PADDING(_pad1_); - - /* Stats updates since the last flush */ - unsigned int stats_updates; - - /* Cached pointers for fast iteration in memcg_rstat_updated() */ - struct memcg_vmstats_percpu *parent; - struct memcg_vmstats *vmstats; }; struct memcg_vmstats {
Commit da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") added two additional pointers to the end of struct memcg_vmstats_percpu with CACHELINE_PADDING to put them in a separate cacheline. This caused the struct size to increase from 1200 to 1280 on my config (80 extra bytes instead of 16). Upon revisiting, the relevant struct members do not need to be on a separate cacheline, they just need to fit in a single one. This is a percpu struct, so there shouldn't be any contention on that cacheline anyway. Move the members to the beginning of the struct and cachealign the first member. Add a comment about the members that need to fit together in a cacheline. The struct size is now 1216 on my config with this change. Fixes: da10d7e140196 ("mm: memcg: optimize parent iteration in memcg_rstat_updated()") Reported-by: Greg Thelen <gthelen@google.com> Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- mm/memcontrol.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)