| Message ID | 20210710003626.3549282-2-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] mm, memcg: add mem_cgroup_disabled checks in vmpressure and swap-related functions | expand |
On Sat, Jul 10, 2021 at 8:36 AM Suren Baghdasaryan <surenb@google.com> wrote: > > Inline mem_cgroup_{charge/uncharge} and mem_cgroup_uncharge_list functions > functions to perform mem_cgroup_disabled static key check inline before > calling the main body of the function. This minimizes the memcg overhead > in the pagefault and exit_mmap paths when memcgs are disabled using > cgroup_disable=memory command-line option. > This change results in ~0.4% overhead reduction when running PFT test > comparing {CONFIG_MEMCG=n} against {CONFIG_MEMCG=y, cgroup_disable=memory} > configurationon on an 8-core ARM64 Android device. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reviewed-by: Shakeel Butt <shakeelb@google.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> But some nits below. > --- > include/linux/memcontrol.h | 28 +++++++++++++++++++++++++--- > mm/memcontrol.c | 29 ++++++++++------------------- > 2 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index bfe5c486f4ad..39fa88051a42 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -693,13 +693,35 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg) > page_counter_read(&memcg->memory); > } > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask); > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask); > +static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask) > +{ > + if (mem_cgroup_disabled()) > + return 0; > + return __mem_cgroup_charge(page, mm, gfp_mask); > +} > + > int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > gfp_t gfp, swp_entry_t entry); > void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry); > > -void mem_cgroup_uncharge(struct page *page); > -void mem_cgroup_uncharge_list(struct list_head *page_list); > +void __mem_cgroup_uncharge(struct page *page); > +static inline void mem_cgroup_uncharge(struct page *page) > +{ > + if (mem_cgroup_disabled()) > + return; > + __mem_cgroup_uncharge(page); > +} > + > +void __mem_cgroup_uncharge_list(struct list_head *page_list); > +static inline void mem_cgroup_uncharge_list(struct list_head *page_list) > +{ > + if (mem_cgroup_disabled()) > + return; > + __mem_cgroup_uncharge_list(page_list); > +} > > void mem_cgroup_migrate(struct page *oldpage, struct page *newpage); > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a228cd51c4bd..cdaf7003b43d 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6701,8 +6701,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root, > atomic_long_read(&parent->memory.children_low_usage))); > } > > -static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > - gfp_t gfp) > +static int charge_memcg(struct page *page, struct mem_cgroup *memcg, gfp_t gfp) > { > unsigned int nr_pages = thp_nr_pages(page); > int ret; > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > } > > /** > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > * @page: page to charge > * @mm: mm context of the victim > * @gfp_mask: reclaim mode > @@ -6736,16 +6735,14 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > * > * Returns 0 on success. Otherwise, an error code is returned. > */ > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask) > { > struct mem_cgroup *memcg; > int ret; > > - if (mem_cgroup_disabled()) > - return 0; > - > memcg = get_mem_cgroup_from_mm(mm); > - ret = __mem_cgroup_charge(page, memcg, gfp_mask); > + ret = charge_memcg(page, memcg, gfp_mask); > css_put(&memcg->css); > > return ret; > @@ -6780,7 +6777,7 @@ int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > memcg = get_mem_cgroup_from_mm(mm); > rcu_read_unlock(); > > - ret = __mem_cgroup_charge(page, memcg, gfp); > + ret = charge_memcg(page, memcg, gfp); > > css_put(&memcg->css); > return ret; > @@ -6916,18 +6913,15 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) > } > > /** > - * mem_cgroup_uncharge - uncharge a page > + * __mem_cgroup_uncharge - uncharge a page > * @page: page to uncharge > * > * Uncharge a page previously charged with mem_cgroup_charge(). The comment here also needs to be updated. mem_cgroup_uncharge() -> __mem_cgroup_uncharge() > */ > -void mem_cgroup_uncharge(struct page *page) > +void __mem_cgroup_uncharge(struct page *page) > { > struct uncharge_gather ug; > > - if (mem_cgroup_disabled()) > - return; > - > /* Don't touch page->lru of any random page, pre-check: */ > if (!page_memcg(page)) > return; > @@ -6938,20 +6932,17 @@ void mem_cgroup_uncharge(struct page *page) > } > > /** > - * mem_cgroup_uncharge_list - uncharge a list of page > + * __mem_cgroup_uncharge_list - uncharge a list of page > * @page_list: list of pages to uncharge > * > * Uncharge a list of pages previously charged with > * mem_cgroup_charge(). Should be __mem_cgroup_charge(). Thanks. > */ > -void mem_cgroup_uncharge_list(struct list_head *page_list) > +void __mem_cgroup_uncharge_list(struct list_head *page_list) > { > struct uncharge_gather ug; > struct page *page; > > - if (mem_cgroup_disabled()) > - return; > - > uncharge_gather_clear(&ug); > list_for_each_entry(page, page_list, lru) > uncharge_page(page, &ug); > -- > 2.32.0.93.g670b81a890-goog >
On Fri 09-07-21 17:36:25, Suren Baghdasaryan wrote: > Inline mem_cgroup_{charge/uncharge} and mem_cgroup_uncharge_list functions > functions to perform mem_cgroup_disabled static key check inline before > calling the main body of the function. This minimizes the memcg overhead > in the pagefault and exit_mmap paths when memcgs are disabled using > cgroup_disable=memory command-line option. > This change results in ~0.4% overhead reduction when running PFT test > comparing {CONFIG_MEMCG=n} against {CONFIG_MEMCG=y, cgroup_disable=memory} > configurationon on an 8-core ARM64 Android device. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reviewed-by: Shakeel Butt <shakeelb@google.com> With doc updated as suggested by Muchun Song Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > include/linux/memcontrol.h | 28 +++++++++++++++++++++++++--- > mm/memcontrol.c | 29 ++++++++++------------------- > 2 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index bfe5c486f4ad..39fa88051a42 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -693,13 +693,35 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg) > page_counter_read(&memcg->memory); > } > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask); > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask); > +static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask) > +{ > + if (mem_cgroup_disabled()) > + return 0; > + return __mem_cgroup_charge(page, mm, gfp_mask); > +} > + > int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > gfp_t gfp, swp_entry_t entry); > void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry); > > -void mem_cgroup_uncharge(struct page *page); > -void mem_cgroup_uncharge_list(struct list_head *page_list); > +void __mem_cgroup_uncharge(struct page *page); > +static inline void mem_cgroup_uncharge(struct page *page) > +{ > + if (mem_cgroup_disabled()) > + return; > + __mem_cgroup_uncharge(page); > +} > + > +void __mem_cgroup_uncharge_list(struct list_head *page_list); > +static inline void mem_cgroup_uncharge_list(struct list_head *page_list) > +{ > + if (mem_cgroup_disabled()) > + return; > + __mem_cgroup_uncharge_list(page_list); > +} > > void mem_cgroup_migrate(struct page *oldpage, struct page *newpage); > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a228cd51c4bd..cdaf7003b43d 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6701,8 +6701,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root, > atomic_long_read(&parent->memory.children_low_usage))); > } > > -static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > - gfp_t gfp) > +static int charge_memcg(struct page *page, struct mem_cgroup *memcg, gfp_t gfp) > { > unsigned int nr_pages = thp_nr_pages(page); > int ret; > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > } > > /** > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > * @page: page to charge > * @mm: mm context of the victim > * @gfp_mask: reclaim mode > @@ -6736,16 +6735,14 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > * > * Returns 0 on success. Otherwise, an error code is returned. > */ > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > + gfp_t gfp_mask) > { > struct mem_cgroup *memcg; > int ret; > > - if (mem_cgroup_disabled()) > - return 0; > - > memcg = get_mem_cgroup_from_mm(mm); > - ret = __mem_cgroup_charge(page, memcg, gfp_mask); > + ret = charge_memcg(page, memcg, gfp_mask); > css_put(&memcg->css); > > return ret; > @@ -6780,7 +6777,7 @@ int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > memcg = get_mem_cgroup_from_mm(mm); > rcu_read_unlock(); > > - ret = __mem_cgroup_charge(page, memcg, gfp); > + ret = charge_memcg(page, memcg, gfp); > > css_put(&memcg->css); > return ret; > @@ -6916,18 +6913,15 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) > } > > /** > - * mem_cgroup_uncharge - uncharge a page > + * __mem_cgroup_uncharge - uncharge a page > * @page: page to uncharge > * > * Uncharge a page previously charged with mem_cgroup_charge(). > */ > -void mem_cgroup_uncharge(struct page *page) > +void __mem_cgroup_uncharge(struct page *page) > { > struct uncharge_gather ug; > > - if (mem_cgroup_disabled()) > - return; > - > /* Don't touch page->lru of any random page, pre-check: */ > if (!page_memcg(page)) > return; > @@ -6938,20 +6932,17 @@ void mem_cgroup_uncharge(struct page *page) > } > > /** > - * mem_cgroup_uncharge_list - uncharge a list of page > + * __mem_cgroup_uncharge_list - uncharge a list of page > * @page_list: list of pages to uncharge > * > * Uncharge a list of pages previously charged with > * mem_cgroup_charge(). > */ > -void mem_cgroup_uncharge_list(struct list_head *page_list) > +void __mem_cgroup_uncharge_list(struct list_head *page_list) > { > struct uncharge_gather ug; > struct page *page; > > - if (mem_cgroup_disabled()) > - return; > - > uncharge_gather_clear(&ug); > list_for_each_entry(page, page_list, lru) > uncharge_page(page, &ug); > -- > 2.32.0.93.g670b81a890-goog
On Mon, Jul 12, 2021 at 12:15 AM Michal Hocko <mhocko@suse.com> wrote: > > On Fri 09-07-21 17:36:25, Suren Baghdasaryan wrote: > > Inline mem_cgroup_{charge/uncharge} and mem_cgroup_uncharge_list functions > > functions to perform mem_cgroup_disabled static key check inline before > > calling the main body of the function. This minimizes the memcg overhead > > in the pagefault and exit_mmap paths when memcgs are disabled using > > cgroup_disable=memory command-line option. > > This change results in ~0.4% overhead reduction when running PFT test > > comparing {CONFIG_MEMCG=n} against {CONFIG_MEMCG=y, cgroup_disable=memory} > > configurationon on an 8-core ARM64 Android device. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Reviewed-by: Shakeel Butt <shakeelb@google.com> > > With doc updated as suggested by Muchun Song > Acked-by: Michal Hocko <mhocko@suse.com> Thanks! Will fix the comment and post v4 later today. > > Thanks! > > > --- > > include/linux/memcontrol.h | 28 +++++++++++++++++++++++++--- > > mm/memcontrol.c | 29 ++++++++++------------------- > > 2 files changed, 35 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index bfe5c486f4ad..39fa88051a42 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -693,13 +693,35 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg) > > page_counter_read(&memcg->memory); > > } > > > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask); > > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask); > > +static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask) > > +{ > > + if (mem_cgroup_disabled()) > > + return 0; > > + return __mem_cgroup_charge(page, mm, gfp_mask); > > +} > > + > > int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > > gfp_t gfp, swp_entry_t entry); > > void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry); > > > > -void mem_cgroup_uncharge(struct page *page); > > -void mem_cgroup_uncharge_list(struct list_head *page_list); > > +void __mem_cgroup_uncharge(struct page *page); > > +static inline void mem_cgroup_uncharge(struct page *page) > > +{ > > + if (mem_cgroup_disabled()) > > + return; > > + __mem_cgroup_uncharge(page); > > +} > > + > > +void __mem_cgroup_uncharge_list(struct list_head *page_list); > > +static inline void mem_cgroup_uncharge_list(struct list_head *page_list) > > +{ > > + if (mem_cgroup_disabled()) > > + return; > > + __mem_cgroup_uncharge_list(page_list); > > +} > > > > void mem_cgroup_migrate(struct page *oldpage, struct page *newpage); > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index a228cd51c4bd..cdaf7003b43d 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -6701,8 +6701,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root, > > atomic_long_read(&parent->memory.children_low_usage))); > > } > > > > -static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > - gfp_t gfp) > > +static int charge_memcg(struct page *page, struct mem_cgroup *memcg, gfp_t gfp) > > { > > unsigned int nr_pages = thp_nr_pages(page); > > int ret; > > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > } > > > > /** > > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > > * @page: page to charge > > * @mm: mm context of the victim > > * @gfp_mask: reclaim mode > > @@ -6736,16 +6735,14 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > * > > * Returns 0 on success. Otherwise, an error code is returned. > > */ > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) > > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask) > > { > > struct mem_cgroup *memcg; > > int ret; > > > > - if (mem_cgroup_disabled()) > > - return 0; > > - > > memcg = get_mem_cgroup_from_mm(mm); > > - ret = __mem_cgroup_charge(page, memcg, gfp_mask); > > + ret = charge_memcg(page, memcg, gfp_mask); > > css_put(&memcg->css); > > > > return ret; > > @@ -6780,7 +6777,7 @@ int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > > memcg = get_mem_cgroup_from_mm(mm); > > rcu_read_unlock(); > > > > - ret = __mem_cgroup_charge(page, memcg, gfp); > > + ret = charge_memcg(page, memcg, gfp); > > > > css_put(&memcg->css); > > return ret; > > @@ -6916,18 +6913,15 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) > > } > > > > /** > > - * mem_cgroup_uncharge - uncharge a page > > + * __mem_cgroup_uncharge - uncharge a page > > * @page: page to uncharge > > * > > * Uncharge a page previously charged with mem_cgroup_charge(). > > */ > > -void mem_cgroup_uncharge(struct page *page) > > +void __mem_cgroup_uncharge(struct page *page) > > { > > struct uncharge_gather ug; > > > > - if (mem_cgroup_disabled()) > > - return; > > - > > /* Don't touch page->lru of any random page, pre-check: */ > > if (!page_memcg(page)) > > return; > > @@ -6938,20 +6932,17 @@ void mem_cgroup_uncharge(struct page *page) > > } > > > > /** > > - * mem_cgroup_uncharge_list - uncharge a list of page > > + * __mem_cgroup_uncharge_list - uncharge a list of page > > * @page_list: list of pages to uncharge > > * > > * Uncharge a list of pages previously charged with > > * mem_cgroup_charge(). > > */ > > -void mem_cgroup_uncharge_list(struct list_head *page_list) > > +void __mem_cgroup_uncharge_list(struct list_head *page_list) > > { > > struct uncharge_gather ug; > > struct page *page; > > > > - if (mem_cgroup_disabled()) > > - return; > > - > > uncharge_gather_clear(&ug); > > list_for_each_entry(page, page_list, lru) > > uncharge_page(page, &ug); > > -- > > 2.32.0.93.g670b81a890-goog > > -- > Michal Hocko > SUSE Labs
On Sat, Jul 10, 2021 at 4:08 AM Muchun Song <songmuchun@bytedance.com> wrote: > > On Sat, Jul 10, 2021 at 8:36 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > Inline mem_cgroup_{charge/uncharge} and mem_cgroup_uncharge_list functions > > functions to perform mem_cgroup_disabled static key check inline before > > calling the main body of the function. This minimizes the memcg overhead > > in the pagefault and exit_mmap paths when memcgs are disabled using > > cgroup_disable=memory command-line option. > > This change results in ~0.4% overhead reduction when running PFT test > > comparing {CONFIG_MEMCG=n} against {CONFIG_MEMCG=y, cgroup_disable=memory} > > configurationon on an 8-core ARM64 Android device. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Reviewed-by: Shakeel Butt <shakeelb@google.com> > > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > > But some nits below. > > > --- > > include/linux/memcontrol.h | 28 +++++++++++++++++++++++++--- > > mm/memcontrol.c | 29 ++++++++++------------------- > > 2 files changed, 35 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index bfe5c486f4ad..39fa88051a42 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -693,13 +693,35 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg) > > page_counter_read(&memcg->memory); > > } > > > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask); > > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask); > > +static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask) > > +{ > > + if (mem_cgroup_disabled()) > > + return 0; > > + return __mem_cgroup_charge(page, mm, gfp_mask); > > +} > > + > > int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > > gfp_t gfp, swp_entry_t entry); > > void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry); > > > > -void mem_cgroup_uncharge(struct page *page); > > -void mem_cgroup_uncharge_list(struct list_head *page_list); > > +void __mem_cgroup_uncharge(struct page *page); > > +static inline void mem_cgroup_uncharge(struct page *page) > > +{ > > + if (mem_cgroup_disabled()) > > + return; > > + __mem_cgroup_uncharge(page); > > +} > > + > > +void __mem_cgroup_uncharge_list(struct list_head *page_list); > > +static inline void mem_cgroup_uncharge_list(struct list_head *page_list) > > +{ > > + if (mem_cgroup_disabled()) > > + return; > > + __mem_cgroup_uncharge_list(page_list); > > +} > > > > void mem_cgroup_migrate(struct page *oldpage, struct page *newpage); > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index a228cd51c4bd..cdaf7003b43d 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -6701,8 +6701,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root, > > atomic_long_read(&parent->memory.children_low_usage))); > > } > > > > -static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > - gfp_t gfp) > > +static int charge_memcg(struct page *page, struct mem_cgroup *memcg, gfp_t gfp) > > { > > unsigned int nr_pages = thp_nr_pages(page); > > int ret; > > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > } > > > > /** > > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > > * @page: page to charge > > * @mm: mm context of the victim > > * @gfp_mask: reclaim mode > > @@ -6736,16 +6735,14 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > * > > * Returns 0 on success. Otherwise, an error code is returned. > > */ > > -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) > > +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, > > + gfp_t gfp_mask) > > { > > struct mem_cgroup *memcg; > > int ret; > > > > - if (mem_cgroup_disabled()) > > - return 0; > > - > > memcg = get_mem_cgroup_from_mm(mm); > > - ret = __mem_cgroup_charge(page, memcg, gfp_mask); > > + ret = charge_memcg(page, memcg, gfp_mask); > > css_put(&memcg->css); > > > > return ret; > > @@ -6780,7 +6777,7 @@ int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, > > memcg = get_mem_cgroup_from_mm(mm); > > rcu_read_unlock(); > > > > - ret = __mem_cgroup_charge(page, memcg, gfp); > > + ret = charge_memcg(page, memcg, gfp); > > > > css_put(&memcg->css); > > return ret; > > @@ -6916,18 +6913,15 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) > > } > > > > /** > > - * mem_cgroup_uncharge - uncharge a page > > + * __mem_cgroup_uncharge - uncharge a page > > * @page: page to uncharge > > * > > * Uncharge a page previously charged with mem_cgroup_charge(). > > The comment here also needs to be updated. > > mem_cgroup_uncharge() -> __mem_cgroup_uncharge() > > > */ > > -void mem_cgroup_uncharge(struct page *page) > > +void __mem_cgroup_uncharge(struct page *page) > > { > > struct uncharge_gather ug; > > > > - if (mem_cgroup_disabled()) > > - return; > > - > > /* Don't touch page->lru of any random page, pre-check: */ > > if (!page_memcg(page)) > > return; > > @@ -6938,20 +6932,17 @@ void mem_cgroup_uncharge(struct page *page) > > } > > > > /** > > - * mem_cgroup_uncharge_list - uncharge a list of page > > + * __mem_cgroup_uncharge_list - uncharge a list of page > > * @page_list: list of pages to uncharge > > * > > * Uncharge a list of pages previously charged with > > * mem_cgroup_charge(). > > Should be __mem_cgroup_charge(). Fixed and posted in v4 at: https://lore.kernel.org/patchwork/project/lkml/list/?series=507943 Thanks! > > Thanks. > > > */ > > -void mem_cgroup_uncharge_list(struct list_head *page_list) > > +void __mem_cgroup_uncharge_list(struct list_head *page_list) > > { > > struct uncharge_gather ug; > > struct page *page; > > > > - if (mem_cgroup_disabled()) > > - return; > > - > > uncharge_gather_clear(&ug); > > list_for_each_entry(page, page_list, lru) > > uncharge_page(page, &ug); > > -- > > 2.32.0.93.g670b81a890-goog > >
On Fri, Jul 09, 2021 at 05:36:25PM -0700, Suren Baghdasaryan wrote: > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > } > > /** > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > * @page: page to charge > * @mm: mm context of the victim > * @gfp_mask: reclaim mode This patch conflicts with the folio work, so I'm just rebasing the folio patches on top of this, and I think this part of the patch is a mistake. We don't want to document the __mem_cgroup_charge() function. That's an implementation detail. This patch should instead have moved the kernel-doc to memcontrol.h and continued to document mem_cgroup_charge().
On Sun, Jul 18, 2021 at 9:56 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Jul 09, 2021 at 05:36:25PM -0700, Suren Baghdasaryan wrote: > > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > } > > > > /** > > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > > * @page: page to charge > > * @mm: mm context of the victim > > * @gfp_mask: reclaim mode > > This patch conflicts with the folio work, so I'm just rebasing the > folio patches on top of this, and I think this part of the patch is a > mistake. We don't want to document the __mem_cgroup_charge() function. > That's an implementation detail. This patch should instead have moved the > kernel-doc to memcontrol.h and continued to document mem_cgroup_charge(). Ack. There was a v4 version of this patch: https://lore.kernel.org/patchwork/patch/1458907 which was picked up by Andrew already. If others agree that documentation should be moved into the header file then I'll gladly post another version. Or I can post a separate patch moving the documentation only. Whatever works best. Andrew, Michal, Johannes, WDYT?
On Sun, Jul 18, 2021 at 02:25:50PM -0700, Suren Baghdasaryan wrote: > On Sun, Jul 18, 2021 at 9:56 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Fri, Jul 09, 2021 at 05:36:25PM -0700, Suren Baghdasaryan wrote: > > > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > > } > > > > > > /** > > > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > > > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > > > * @page: page to charge > > > * @mm: mm context of the victim > > > * @gfp_mask: reclaim mode > > > > This patch conflicts with the folio work, so I'm just rebasing the > > folio patches on top of this, and I think this part of the patch is a > > mistake. We don't want to document the __mem_cgroup_charge() function. > > That's an implementation detail. This patch should instead have moved the > > kernel-doc to memcontrol.h and continued to document mem_cgroup_charge(). > > Ack. > There was a v4 version of this patch: > https://lore.kernel.org/patchwork/patch/1458907 which was picked up by > Andrew already. If others agree that documentation should be moved > into the header file then I'll gladly post another version. Or I can > post a separate patch moving the documentation only. Whatever works > best. Andrew, Michal, Johannes, WDYT? At this point, I've moved the documentation as part of the folio patch. I'd rather not redo that patch again ...
On Sun, Jul 18, 2021 at 2:30 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Jul 18, 2021 at 02:25:50PM -0700, Suren Baghdasaryan wrote: > > On Sun, Jul 18, 2021 at 9:56 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Fri, Jul 09, 2021 at 05:36:25PM -0700, Suren Baghdasaryan wrote: > > > > @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, > > > > } > > > > > > > > /** > > > > - * mem_cgroup_charge - charge a newly allocated page to a cgroup > > > > + * __mem_cgroup_charge - charge a newly allocated page to a cgroup > > > > * @page: page to charge > > > > * @mm: mm context of the victim > > > > * @gfp_mask: reclaim mode > > > > > > This patch conflicts with the folio work, so I'm just rebasing the > > > folio patches on top of this, and I think this part of the patch is a > > > mistake. We don't want to document the __mem_cgroup_charge() function. > > > That's an implementation detail. This patch should instead have moved the > > > kernel-doc to memcontrol.h and continued to document mem_cgroup_charge(). > > > > Ack. > > There was a v4 version of this patch: > > https://lore.kernel.org/patchwork/patch/1458907 which was picked up by > > Andrew already. If others agree that documentation should be moved > > into the header file then I'll gladly post another version. Or I can > > post a separate patch moving the documentation only. Whatever works > > best. Andrew, Michal, Johannes, WDYT? > > At this point, I've moved the documentation as part of the folio patch. > I'd rather not redo that patch again ... Ok. If you need me to redo anything please let me know.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index bfe5c486f4ad..39fa88051a42 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -693,13 +693,35 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg) page_counter_read(&memcg->memory); } -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask); +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, + gfp_t gfp_mask); +static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm, + gfp_t gfp_mask) +{ + if (mem_cgroup_disabled()) + return 0; + return __mem_cgroup_charge(page, mm, gfp_mask); +} + int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, gfp_t gfp, swp_entry_t entry); void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry); -void mem_cgroup_uncharge(struct page *page); -void mem_cgroup_uncharge_list(struct list_head *page_list); +void __mem_cgroup_uncharge(struct page *page); +static inline void mem_cgroup_uncharge(struct page *page) +{ + if (mem_cgroup_disabled()) + return; + __mem_cgroup_uncharge(page); +} + +void __mem_cgroup_uncharge_list(struct list_head *page_list); +static inline void mem_cgroup_uncharge_list(struct list_head *page_list) +{ + if (mem_cgroup_disabled()) + return; + __mem_cgroup_uncharge_list(page_list); +} void mem_cgroup_migrate(struct page *oldpage, struct page *newpage); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a228cd51c4bd..cdaf7003b43d 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6701,8 +6701,7 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root, atomic_long_read(&parent->memory.children_low_usage))); } -static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, - gfp_t gfp) +static int charge_memcg(struct page *page, struct mem_cgroup *memcg, gfp_t gfp) { unsigned int nr_pages = thp_nr_pages(page); int ret; @@ -6723,7 +6722,7 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, } /** - * mem_cgroup_charge - charge a newly allocated page to a cgroup + * __mem_cgroup_charge - charge a newly allocated page to a cgroup * @page: page to charge * @mm: mm context of the victim * @gfp_mask: reclaim mode @@ -6736,16 +6735,14 @@ static int __mem_cgroup_charge(struct page *page, struct mem_cgroup *memcg, * * Returns 0 on success. Otherwise, an error code is returned. */ -int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) +int __mem_cgroup_charge(struct page *page, struct mm_struct *mm, + gfp_t gfp_mask) { struct mem_cgroup *memcg; int ret; - if (mem_cgroup_disabled()) - return 0; - memcg = get_mem_cgroup_from_mm(mm); - ret = __mem_cgroup_charge(page, memcg, gfp_mask); + ret = charge_memcg(page, memcg, gfp_mask); css_put(&memcg->css); return ret; @@ -6780,7 +6777,7 @@ int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm, memcg = get_mem_cgroup_from_mm(mm); rcu_read_unlock(); - ret = __mem_cgroup_charge(page, memcg, gfp); + ret = charge_memcg(page, memcg, gfp); css_put(&memcg->css); return ret; @@ -6916,18 +6913,15 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) } /** - * mem_cgroup_uncharge - uncharge a page + * __mem_cgroup_uncharge - uncharge a page * @page: page to uncharge * * Uncharge a page previously charged with mem_cgroup_charge(). */ -void mem_cgroup_uncharge(struct page *page) +void __mem_cgroup_uncharge(struct page *page) { struct uncharge_gather ug; - if (mem_cgroup_disabled()) - return; - /* Don't touch page->lru of any random page, pre-check: */ if (!page_memcg(page)) return; @@ -6938,20 +6932,17 @@ void mem_cgroup_uncharge(struct page *page) } /** - * mem_cgroup_uncharge_list - uncharge a list of page + * __mem_cgroup_uncharge_list - uncharge a list of page * @page_list: list of pages to uncharge * * Uncharge a list of pages previously charged with * mem_cgroup_charge(). */ -void mem_cgroup_uncharge_list(struct list_head *page_list) +void __mem_cgroup_uncharge_list(struct list_head *page_list) { struct uncharge_gather ug; struct page *page; - if (mem_cgroup_disabled()) - return; - uncharge_gather_clear(&ug); list_for_each_entry(page, page_list, lru) uncharge_page(page, &ug);
Inline mem_cgroup_{charge/uncharge} and mem_cgroup_uncharge_list functions functions to perform mem_cgroup_disabled static key check inline before calling the main body of the function. This minimizes the memcg overhead in the pagefault and exit_mmap paths when memcgs are disabled using cgroup_disable=memory command-line option. This change results in ~0.4% overhead reduction when running PFT test comparing {CONFIG_MEMCG=n} against {CONFIG_MEMCG=y, cgroup_disable=memory} configurationon on an 8-core ARM64 Android device. Signed-off-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> --- include/linux/memcontrol.h | 28 +++++++++++++++++++++++++--- mm/memcontrol.c | 29 ++++++++++------------------- 2 files changed, 35 insertions(+), 22 deletions(-)