| Message ID | 1560376609-113689-3-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make deferred split shrinker memcg aware | expand |
On Thu, Jun 13, 2019 at 05:56:47AM +0800, Yang Shi wrote: > The later patch would make THP deferred split shrinker memcg aware, but > it needs page->mem_cgroup information in THP destructor, which is called > after mem_cgroup_uncharge() now. > > So, move mem_cgroup_uncharge() from __page_cache_release() to compound > page destructor, which is called by both THP and other compound pages > except HugeTLB. And call it in __put_single_page() for single order > page. If I read the patch correctly, it will change behaviour for pages with NULL_COMPOUND_DTOR. Have you considered it? Are you sure it will not break anything?
On 6/13/19 4:39 AM, Kirill A. Shutemov wrote: > On Thu, Jun 13, 2019 at 05:56:47AM +0800, Yang Shi wrote: >> The later patch would make THP deferred split shrinker memcg aware, but >> it needs page->mem_cgroup information in THP destructor, which is called >> after mem_cgroup_uncharge() now. >> >> So, move mem_cgroup_uncharge() from __page_cache_release() to compound >> page destructor, which is called by both THP and other compound pages >> except HugeTLB. And call it in __put_single_page() for single order >> page. > > If I read the patch correctly, it will change behaviour for pages with > NULL_COMPOUND_DTOR. Have you considered it? Are you sure it will not break > anything? So far a quick search shows NULL_COMPOUND_DTOR is not used by any type of compound page. The HugeTLB code sets destructor to NULL_COMPOUND_DTOR when freeing hugetlb pages via hugetlb specific destructor. The prep_new_page() would call prep_compound_page() if __GFP_COMP is used, which sets dtor to COMPOUND_PAGE_DTOR by default. Just hugetlb and THP set their specific dtors. And, it looks __put_compound_page() doesn't check if dtor is NULL or not at all. >
On 6/13/19 10:13 AM, Yang Shi wrote: > > > On 6/13/19 4:39 AM, Kirill A. Shutemov wrote: >> On Thu, Jun 13, 2019 at 05:56:47AM +0800, Yang Shi wrote: >>> The later patch would make THP deferred split shrinker memcg aware, but >>> it needs page->mem_cgroup information in THP destructor, which is >>> called >>> after mem_cgroup_uncharge() now. >>> >>> So, move mem_cgroup_uncharge() from __page_cache_release() to compound >>> page destructor, which is called by both THP and other compound pages >>> except HugeTLB. And call it in __put_single_page() for single order >>> page. >> >> If I read the patch correctly, it will change behaviour for pages with >> NULL_COMPOUND_DTOR. Have you considered it? Are you sure it will not >> break >> anything? > Hi Kirill, Did this solve your concern? Any more comments on this series? Thanks, Yang > So far a quick search shows NULL_COMPOUND_DTOR is not used by any type > of compound page. The HugeTLB code sets destructor to > NULL_COMPOUND_DTOR when freeing hugetlb pages via hugetlb specific > destructor. > > The prep_new_page() would call prep_compound_page() if __GFP_COMP is > used, which sets dtor to COMPOUND_PAGE_DTOR by default. Just hugetlb > and THP set their specific dtors. > > And, it looks __put_compound_page() doesn't check if dtor is NULL or > not at all. > >> >
On Mon, Jun 24, 2019 at 09:54:05AM -0700, Yang Shi wrote: > > > On 6/13/19 10:13 AM, Yang Shi wrote: > > > > > > On 6/13/19 4:39 AM, Kirill A. Shutemov wrote: > > > On Thu, Jun 13, 2019 at 05:56:47AM +0800, Yang Shi wrote: > > > > The later patch would make THP deferred split shrinker memcg aware, but > > > > it needs page->mem_cgroup information in THP destructor, which > > > > is called > > > > after mem_cgroup_uncharge() now. > > > > > > > > So, move mem_cgroup_uncharge() from __page_cache_release() to compound > > > > page destructor, which is called by both THP and other compound pages > > > > except HugeTLB. And call it in __put_single_page() for single order > > > > page. > > > > > > If I read the patch correctly, it will change behaviour for pages with > > > NULL_COMPOUND_DTOR. Have you considered it? Are you sure it will not > > > break > > > anything? > > > > Hi Kirill, > > Did this solve your concern? Any more comments on this series? Everyting looks good now. You can use my Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> for the series.
On 6/25/19 2:35 AM, Kirill A. Shutemov wrote: > On Mon, Jun 24, 2019 at 09:54:05AM -0700, Yang Shi wrote: >> >> On 6/13/19 10:13 AM, Yang Shi wrote: >>> >>> On 6/13/19 4:39 AM, Kirill A. Shutemov wrote: >>>> On Thu, Jun 13, 2019 at 05:56:47AM +0800, Yang Shi wrote: >>>>> The later patch would make THP deferred split shrinker memcg aware, but >>>>> it needs page->mem_cgroup information in THP destructor, which >>>>> is called >>>>> after mem_cgroup_uncharge() now. >>>>> >>>>> So, move mem_cgroup_uncharge() from __page_cache_release() to compound >>>>> page destructor, which is called by both THP and other compound pages >>>>> except HugeTLB. And call it in __put_single_page() for single order >>>>> page. >>>> If I read the patch correctly, it will change behaviour for pages with >>>> NULL_COMPOUND_DTOR. Have you considered it? Are you sure it will not >>>> break >>>> anything? >> Hi Kirill, >> >> Did this solve your concern? Any more comments on this series? > Everyting looks good now. You can use my > > Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > for the series. Thanks! >
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index a82104a..7f27f4e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -623,6 +623,7 @@ static void bad_page(struct page *page, const char *reason, void free_compound_page(struct page *page) { + mem_cgroup_uncharge(page); __free_pages_ok(page, compound_order(page)); } diff --git a/mm/swap.c b/mm/swap.c index 3a75722..982bd79 100644 --- a/mm/swap.c +++ b/mm/swap.c @@ -70,12 +70,12 @@ static void __page_cache_release(struct page *page) spin_unlock_irqrestore(&pgdat->lru_lock, flags); } __ClearPageWaiters(page); - mem_cgroup_uncharge(page); } static void __put_single_page(struct page *page) { __page_cache_release(page); + mem_cgroup_uncharge(page); free_unref_page(page); }
The later patch would make THP deferred split shrinker memcg aware, but it needs page->mem_cgroup information in THP destructor, which is called after mem_cgroup_uncharge() now. So, move mem_cgroup_uncharge() from __page_cache_release() to compound page destructor, which is called by both THP and other compound pages except HugeTLB. And call it in __put_single_page() for single order page. Suggested-by: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com> Cc: Kirill Tkhai <ktkhai@virtuozzo.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Hugh Dickins <hughd@google.com> Cc: Shakeel Butt <shakeelb@google.com> Cc: David Rientjes <rientjes@google.com> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- mm/page_alloc.c | 1 + mm/swap.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)