| Message ID | 20200608230654.828134-2-guro@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | The new cgroup slab memory controller | expand |
On Mon, Jun 8, 2020 at 4:07 PM Roman Gushchin <guro@fb.com> wrote: > > To convert memcg and lruvec slab counters to bytes there must be > a way to change these counters without touching node counters. > Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state(). > > Signed-off-by: Roman Gushchin <guro@fb.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > --- > include/linux/memcontrol.h | 17 +++++++++++++++ > mm/memcontrol.c | 43 +++++++++++++++++++++----------------- > 2 files changed, 41 insertions(+), 19 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index bbf624a7f5a6..93dbc7f9d8b8 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -679,11 +679,23 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > return x; > } > > +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > + int val); > void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > int val); > void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val); > void mod_memcg_obj_state(void *p, int idx, int val); > > +static inline void mod_memcg_lruvec_state(struct lruvec *lruvec, > + enum node_stat_item idx, int val) Is this function used in later patches? Any benefit introducing it here instead of in the patch where it is used for the first time? > +{ > + unsigned long flags; > + > + local_irq_save(flags); > + __mod_memcg_lruvec_state(lruvec, idx, val); > + local_irq_restore(flags); > +} > + [...]
On Tue, Jun 16, 2020 at 06:52:09PM -0700, Shakeel Butt wrote: > On Mon, Jun 8, 2020 at 4:07 PM Roman Gushchin <guro@fb.com> wrote: > > > > To convert memcg and lruvec slab counters to bytes there must be > > a way to change these counters without touching node counters. > > Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state(). > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > --- > > include/linux/memcontrol.h | 17 +++++++++++++++ > > mm/memcontrol.c | 43 +++++++++++++++++++++----------------- > > 2 files changed, 41 insertions(+), 19 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index bbf624a7f5a6..93dbc7f9d8b8 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -679,11 +679,23 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > > return x; > > } > > > > +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > + int val); > > void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > int val); > > void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val); > > void mod_memcg_obj_state(void *p, int idx, int val); > > > > +static inline void mod_memcg_lruvec_state(struct lruvec *lruvec, > > + enum node_stat_item idx, int val) > > Is this function used in later patches? Any benefit introducing it > here instead of in the patch where it is used for the first time? Yes, it's used in "mm: memcg/slab: charge individual slab objects instead of pages". It's a fairly large patchset with many internal dependencies, so there is always a trade-off between putting everything into a single patch, which is hard to review, and splitting out some changes, which make not much sense without seeing the whole picture. In this particular case splitting out a formal and easy-to-verify change makes the actual non-trivial patch smaller and hopefully easier for a review. But of course it's all subjective. Thanks!
On Tue, Jun 16, 2020 at 7:50 PM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Jun 16, 2020 at 06:52:09PM -0700, Shakeel Butt wrote: > > On Mon, Jun 8, 2020 at 4:07 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > To convert memcg and lruvec slab counters to bytes there must be > > > a way to change these counters without touching node counters. > > > Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state(). > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > > --- > > > include/linux/memcontrol.h | 17 +++++++++++++++ > > > mm/memcontrol.c | 43 +++++++++++++++++++++----------------- > > > 2 files changed, 41 insertions(+), 19 deletions(-) > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > index bbf624a7f5a6..93dbc7f9d8b8 100644 > > > --- a/include/linux/memcontrol.h > > > +++ b/include/linux/memcontrol.h > > > @@ -679,11 +679,23 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > > > return x; > > > } > > > > > > +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > > + int val); > > > void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > > int val); > > > void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val); > > > void mod_memcg_obj_state(void *p, int idx, int val); > > > > > > +static inline void mod_memcg_lruvec_state(struct lruvec *lruvec, > > > + enum node_stat_item idx, int val) > > > > Is this function used in later patches? Any benefit introducing it > > here instead of in the patch where it is used for the first time? > > Yes, it's used in "mm: memcg/slab: charge individual slab objects instead of pages". > > It's a fairly large patchset with many internal dependencies, so there is > always a trade-off between putting everything into a single patch, which is > hard to review, and splitting out some changes, which make not much sense > without seeing the whole picture. > > In this particular case splitting out a formal and easy-to-verify change makes > the actual non-trivial patch smaller and hopefully easier for a review. > > But of course it's all subjective. > > Thanks! I am fine with that. Reviewed-by: Shakeel Butt <shakeelb@google.com>
On Tue, Jun 16, 2020 at 07:59:40PM -0700, Shakeel Butt wrote: > On Tue, Jun 16, 2020 at 7:50 PM Roman Gushchin <guro@fb.com> wrote: > > > > On Tue, Jun 16, 2020 at 06:52:09PM -0700, Shakeel Butt wrote: > > > On Mon, Jun 8, 2020 at 4:07 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > > > To convert memcg and lruvec slab counters to bytes there must be > > > > a way to change these counters without touching node counters. > > > > Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state(). > > > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > > > --- > > > > include/linux/memcontrol.h | 17 +++++++++++++++ > > > > mm/memcontrol.c | 43 +++++++++++++++++++++----------------- > > > > 2 files changed, 41 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > > index bbf624a7f5a6..93dbc7f9d8b8 100644 > > > > --- a/include/linux/memcontrol.h > > > > +++ b/include/linux/memcontrol.h > > > > @@ -679,11 +679,23 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, > > > > return x; > > > > } > > > > > > > > +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > > > + int val); > > > > void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, > > > > int val); > > > > void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val); > > > > void mod_memcg_obj_state(void *p, int idx, int val); > > > > > > > > +static inline void mod_memcg_lruvec_state(struct lruvec *lruvec, > > > > + enum node_stat_item idx, int val) > > > > > > Is this function used in later patches? Any benefit introducing it > > > here instead of in the patch where it is used for the first time? > > > > Yes, it's used in "mm: memcg/slab: charge individual slab objects instead of pages". > > > > It's a fairly large patchset with many internal dependencies, so there is > > always a trade-off between putting everything into a single patch, which is > > hard to review, and splitting out some changes, which make not much sense > > without seeing the whole picture. > > > > In this particular case splitting out a formal and easy-to-verify change makes > > the actual non-trivial patch smaller and hopefully easier for a review. > > > > But of course it's all subjective. > > > > Thanks! > > I am fine with that. > > Reviewed-by: Shakeel Butt <shakeelb@google.com> Thank you! Appreciate it!
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index bbf624a7f5a6..93dbc7f9d8b8 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -679,11 +679,23 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, return x; } +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, + int val); void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, int val); void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val); void mod_memcg_obj_state(void *p, int idx, int val); +static inline void mod_memcg_lruvec_state(struct lruvec *lruvec, + enum node_stat_item idx, int val) +{ + unsigned long flags; + + local_irq_save(flags); + __mod_memcg_lruvec_state(lruvec, idx, val); + local_irq_restore(flags); +} + static inline void mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, int val) { @@ -1057,6 +1069,11 @@ static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec, return node_page_state(lruvec_pgdat(lruvec), idx); } +static inline void __mod_memcg_lruvec_state(struct lruvec *lruvec, + enum node_stat_item idx, int val) +{ +} + static inline void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, int val) { diff --git a/mm/memcontrol.c b/mm/memcontrol.c index bbda19c13c19..e8a91e98556b 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -713,30 +713,13 @@ parent_nodeinfo(struct mem_cgroup_per_node *pn, int nid) return mem_cgroup_nodeinfo(parent, nid); } -/** - * __mod_lruvec_state - update lruvec memory statistics - * @lruvec: the lruvec - * @idx: the stat item - * @val: delta to add to the counter, can be negative - * - * The lruvec is the intersection of the NUMA node and a cgroup. This - * function updates the all three counters that are affected by a - * change of state at this level: per-node, per-cgroup, per-lruvec. - */ -void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, - int val) +void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, + int val) { - pg_data_t *pgdat = lruvec_pgdat(lruvec); struct mem_cgroup_per_node *pn; struct mem_cgroup *memcg; long x; - /* Update node */ - __mod_node_page_state(pgdat, idx, val); - - if (mem_cgroup_disabled()) - return; - pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec); memcg = pn->memcg; @@ -748,6 +731,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, x = val + __this_cpu_read(pn->lruvec_stat_cpu->count[idx]); if (unlikely(abs(x) > MEMCG_CHARGE_BATCH)) { + pg_data_t *pgdat = lruvec_pgdat(lruvec); struct mem_cgroup_per_node *pi; for (pi = pn; pi; pi = parent_nodeinfo(pi, pgdat->node_id)) @@ -757,6 +741,27 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, __this_cpu_write(pn->lruvec_stat_cpu->count[idx], x); } +/** + * __mod_lruvec_state - update lruvec memory statistics + * @lruvec: the lruvec + * @idx: the stat item + * @val: delta to add to the counter, can be negative + * + * The lruvec is the intersection of the NUMA node and a cgroup. This + * function updates the all three counters that are affected by a + * change of state at this level: per-node, per-cgroup, per-lruvec. + */ +void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx, + int val) +{ + /* Update node */ + __mod_node_page_state(lruvec_pgdat(lruvec), idx, val); + + /* Update memcg and lruvec */ + if (!mem_cgroup_disabled()) + __mod_memcg_lruvec_state(lruvec, idx, val); +} + void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val) { pg_data_t *pgdat = page_pgdat(virt_to_page(p));