| Message ID | 2053ac36-74df-b05e-d1ce-36f69dde2a47@ascade.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
TSUKADA Koutaro <tsukada@ascade.co.jp> writes: > On 2018/05/19 2:51, Punit Agrawal wrote: >> Punit Agrawal <punit.agrawal@arm.com> writes: >> >>> Tsukada-san, >>> >>> I am not familiar with memcg so can't comment about whether the patchset >>> is the right way to solve the problem outlined in the cover letter but >>> had a couple of comments about this patch. >>> >>> TSUKADA Koutaro <tsukada@ascade.co.jp> writes: >>> >>>> The current memcg implementation assumes that the compound page is THP. >>>> In order to be able to charge surplus hugepage, we use compound_order. >>>> >>>> Signed-off-by: TSUKADA Koutaro <tsukada@ascade.co.jp> >>> >>> Please move this before Patch 1/7. This is to prevent wrong accounting >>> of pages to memcg for size != PMD_SIZE. >> >> I just noticed that the default state is off so the change isn't enabled >> until the sysfs node is exposed in the next patch. Please ignore this >> comment. >> >> One below still applies. >> >>> >>>> --- >>>> memcontrol.c | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>>> index 2bd3df3..a8f1ff8 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -4483,7 +4483,7 @@ static int mem_cgroup_move_account(struct page *page, >>>> struct mem_cgroup *to) >>>> { >>>> unsigned long flags; >>>> - unsigned int nr_pages = compound ? hpage_nr_pages(page) : 1; >>>> + unsigned int nr_pages = compound ? (1 << compound_order(page)) : 1; >>> >>> Instead of replacing calls to hpage_nr_pages(), is it possible to modify >>> it to do the calculation? > > Thank you for review my code and please just call me Tsukada. > > I think it is possible to modify the inside of itself rather than > replacing the call to hpage_nr_pages(). > > Inferring from the processing that hpage_nr_pages() desires, I thought > that the definition of hpage_nr_pages() could be moved outside the > CONFIG_TRANSPARENT_HUGEPAGE. It seems that THP and HugeTLBfs can be > handled correctly because compound_order() is judged by seeing whether it > is PageHead or not. > > Also, I would like to use compound_order() inside hpage_nr_pages(), but > since huge_mm.h is included before mm.h where compound_order() is defined, > move hpage_nr_pages to mm.h. > > Instead of patch 3/7, are the following patches implementing what you > intended? > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index a8a1262..1186ab7 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -204,12 +204,6 @@ static inline spinlock_t *pud_trans_huge_lock(pud_t *pud, > else > return NULL; > } > -static inline int hpage_nr_pages(struct page *page) > -{ > - if (unlikely(PageTransHuge(page))) > - return HPAGE_PMD_NR; > - return 1; > -} > > struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr, > pmd_t *pmd, int flags); > @@ -254,8 +248,6 @@ static inline bool thp_migration_supported(void) > #define HPAGE_PUD_MASK ({ BUILD_BUG(); 0; }) > #define HPAGE_PUD_SIZE ({ BUILD_BUG(); 0; }) > > -#define hpage_nr_pages(x) 1 > - > static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma) > { > return false; > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 1ac1f06..082f2ee 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -673,6 +673,12 @@ static inline unsigned int compound_order(struct page *page) > return page[1].compound_order; > } > > +static inline int hpage_nr_pages(struct page *page) > +{ > + VM_BUG_ON_PAGE(PageTail(page), page); > + return (1 << compound_order(page)); > +} > + > static inline void set_compound_order(struct page *page, unsigned int order) > { > page[1].compound_order = order; That looks a lot better. Thanks for giving it a go.
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index a8a1262..1186ab7 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -204,12 +204,6 @@ static inline spinlock_t *pud_trans_huge_lock(pud_t *pud, else return NULL; } -static inline int hpage_nr_pages(struct page *page) -{ - if (unlikely(PageTransHuge(page))) - return HPAGE_PMD_NR; - return 1; -} struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmd, int flags); @@ -254,8 +248,6 @@ static inline bool thp_migration_supported(void) #define HPAGE_PUD_MASK ({ BUILD_BUG(); 0; }) #define HPAGE_PUD_SIZE ({ BUILD_BUG(); 0; }) -#define hpage_nr_pages(x) 1 - static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma) { return false; diff --git a/include/linux/mm.h b/include/linux/mm.h index 1ac1f06..082f2ee 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -673,6 +673,12 @@ static inline unsigned int compound_order(struct page *page) return page[1].compound_order; } +static inline int hpage_nr_pages(struct page *page) +{ + VM_BUG_ON_PAGE(PageTail(page), page); + return (1 << compound_order(page)); +} + static inline void set_compound_order(struct page *page, unsigned int order) { page[1].compound_order = order;