| Message ID | 20220622170627.19786-15-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup patches for huge_memory | expand |
On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote: > The current comment is confusing because if global or memcg deferred list > in the second tail page is occupied by compound_head, why we still use > page[2].deferred_list here? I think it wants to say that Global or memcg > deferred list in the first tail page is occupied by compound_mapcount and > compound_pincount so we use the second tail page's deferred_list instead. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > include/linux/huge_mm.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index 12b297f9951d..2e8062b3417a 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void) > static inline struct list_head *page_deferred_list(struct page *page) > { > /* > - * Global or memcg deferred list in the second tail pages is > - * occupied by compound_head. > + * Global or memcg deferred list in the first tail page is > + * occupied by compound_mapcount and compound_pincount. > */ The structure of "struct page" seems to have told us the information that we resue the 2nd tail page to be used as deferred_list. I am not sure the value of those comments. Maybe better to remove them? Thanks. > return &page[2].deferred_list; > } > -- > 2.23.0 > >
On 2022/6/23 15:24, Muchun Song wrote: > On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote: >> The current comment is confusing because if global or memcg deferred list >> in the second tail page is occupied by compound_head, why we still use >> page[2].deferred_list here? I think it wants to say that Global or memcg >> deferred list in the first tail page is occupied by compound_mapcount and >> compound_pincount so we use the second tail page's deferred_list instead. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> include/linux/huge_mm.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >> index 12b297f9951d..2e8062b3417a 100644 >> --- a/include/linux/huge_mm.h >> +++ b/include/linux/huge_mm.h >> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void) >> static inline struct list_head *page_deferred_list(struct page *page) >> { >> /* >> - * Global or memcg deferred list in the second tail pages is >> - * occupied by compound_head. >> + * Global or memcg deferred list in the first tail page is >> + * occupied by compound_mapcount and compound_pincount. >> */ > > The structure of "struct page" seems to have told us the information that > we resue the 2nd tail page to be used as deferred_list. I am not sure the Yes, it does. > value of those comments. Maybe better to remove them? IMHO above comment tries to tell us why deferred list in the second tail page is used instead of first tail page. But it should be fine to remove the above comments as they don't seem to provide much info (thought I'm not really sure). Thanks. > > Thanks. > >> return &page[2].deferred_list; >> } >> -- >> 2.23.0 >> >> > . >
On 23 Jun 20:26, Miaohe Lin wrote: > On 2022/6/23 15:24, Muchun Song wrote: > > On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote: > >> The current comment is confusing because if global or memcg deferred list > >> in the second tail page is occupied by compound_head, why we still use > >> page[2].deferred_list here? I think it wants to say that Global or memcg > >> deferred list in the first tail page is occupied by compound_mapcount and > >> compound_pincount so we use the second tail page's deferred_list instead. > >> > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> --- > >> include/linux/huge_mm.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > >> index 12b297f9951d..2e8062b3417a 100644 > >> --- a/include/linux/huge_mm.h > >> +++ b/include/linux/huge_mm.h > >> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void) > >> static inline struct list_head *page_deferred_list(struct page *page) > >> { > >> /* > >> - * Global or memcg deferred list in the second tail pages is > >> - * occupied by compound_head. > >> + * Global or memcg deferred list in the first tail page is > >> + * occupied by compound_mapcount and compound_pincount. > >> */ > > > > The structure of "struct page" seems to have told us the information that > > we resue the 2nd tail page to be used as deferred_list. I am not sure the > > Yes, it does. > > > value of those comments. Maybe better to remove them? > > IMHO above comment tries to tell us why deferred list in the second tail page is used > instead of first tail page. But it should be fine to remove the above comments as they > don't seem to provide much info (thought I'm not really sure). > > Thanks. > Just a suggestion - feel free to disregard. Maybe we don't need to repeat the comments in struct page, but maybe a "see organization of tail pages of compound page in "struct page" definition" would at least point new people to where this magic 2 comes from. Maybe an obvious place to check after you're familiar with overloading struct page data for compound pages - but IMO it's not obvious for newcomers. > > > > Thanks. > > > >> return &page[2].deferred_list; > >> } > >> -- > >> 2.23.0 > >> > >> > > . > > >
On 2022/6/25 1:09, Zach O'Keefe wrote: > On 23 Jun 20:26, Miaohe Lin wrote: >> On 2022/6/23 15:24, Muchun Song wrote: >>> On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote: >>>> The current comment is confusing because if global or memcg deferred list >>>> in the second tail page is occupied by compound_head, why we still use >>>> page[2].deferred_list here? I think it wants to say that Global or memcg >>>> deferred list in the first tail page is occupied by compound_mapcount and >>>> compound_pincount so we use the second tail page's deferred_list instead. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> include/linux/huge_mm.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>>> index 12b297f9951d..2e8062b3417a 100644 >>>> --- a/include/linux/huge_mm.h >>>> +++ b/include/linux/huge_mm.h >>>> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void) >>>> static inline struct list_head *page_deferred_list(struct page *page) >>>> { >>>> /* >>>> - * Global or memcg deferred list in the second tail pages is >>>> - * occupied by compound_head. >>>> + * Global or memcg deferred list in the first tail page is >>>> + * occupied by compound_mapcount and compound_pincount. >>>> */ >>> >>> The structure of "struct page" seems to have told us the information that >>> we resue the 2nd tail page to be used as deferred_list. I am not sure the >> >> Yes, it does. >> >>> value of those comments. Maybe better to remove them? >> >> IMHO above comment tries to tell us why deferred list in the second tail page is used >> instead of first tail page. But it should be fine to remove the above comments as they >> don't seem to provide much info (thought I'm not really sure). >> >> Thanks. >> > > Just a suggestion - feel free to disregard. Maybe we don't need to repeat the > comments in struct page, but maybe a "see organization of tail pages of compound > page in "struct page" definition" would at least point new people to where this > magic 2 comes from. Maybe an obvious place to check after you're familiar with > overloading struct page data for compound pages - but IMO it's not obvious for > newcomers. This is a good suggestion to me. Thanks! > >>> >>> Thanks. >>> >>>> return &page[2].deferred_list; >>>> } >>>> -- >>>> 2.23.0 >>>> >>>> >>> . >>> >> > . >
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index 12b297f9951d..2e8062b3417a 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void) static inline struct list_head *page_deferred_list(struct page *page) { /* - * Global or memcg deferred list in the second tail pages is - * occupied by compound_head. + * Global or memcg deferred list in the first tail page is + * occupied by compound_mapcount and compound_pincount. */ return &page[2].deferred_list; }
The current comment is confusing because if global or memcg deferred list in the second tail page is occupied by compound_head, why we still use page[2].deferred_list here? I think it wants to say that Global or memcg deferred list in the first tail page is occupied by compound_mapcount and compound_pincount so we use the second tail page's deferred_list instead. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- include/linux/huge_mm.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)