| Message ID | 20220607144157.36411-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_alloc: make calling prep_compound_head more reliable | expand |
Let's cc Joao. On Tue, 7 Jun 2022 22:41:57 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > compound_pincount_ptr is stored at first tail page instead of second tail > page now. "now"? Some identifiable commit did this? > And if it or some other field changes again in the future, data > overwritten might happen. Calling prep_compound_head() outside the loop > to prevent such possible issue. No functional change intended. > > ... > > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head, > __init_zone_device_page(page, pfn, zone_idx, nid, pgmap); > prep_compound_tail(head, pfn - head_pfn); > set_page_count(page, 0); > - > - /* > - * The first tail page stores compound_mapcount_ptr() and > - * compound_order() and the second tail page stores > - * compound_pincount_ptr(). Call prep_compound_head() after > - * the first and second tail pages have been initialized to > - * not have the data overwritten. > - */ > - if (pfn == head_pfn + 2) > - prep_compound_head(head, order); > } > + prep_compound_head(head, order); > } > > void __ref memmap_init_zone_device(struct zone *zone,
On 6/7/22 19:32, Andrew Morton wrote: > > Let's cc Joao. > > On Tue, 7 Jun 2022 22:41:57 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > >> compound_pincount_ptr is stored at first tail page instead of second tail >> page now. > > "now"? Some identifiable commit did this? > I think this was in: commit5232c63f46fd ("mm: Make compound_pincount always available") >> And if it or some other field changes again in the future, data >> overwritten might happen. Calling prep_compound_head() outside the loop >> to prevent such possible issue. No functional change intended. >> >> ... >> >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head, >> __init_zone_device_page(page, pfn, zone_idx, nid, pgmap); >> prep_compound_tail(head, pfn - head_pfn); >> set_page_count(page, 0); >> - >> - /* >> - * The first tail page stores compound_mapcount_ptr() and >> - * compound_order() and the second tail page stores >> - * compound_pincount_ptr(). Call prep_compound_head() after >> - * the first and second tail pages have been initialized to >> - * not have the data overwritten. >> - */ >> - if (pfn == head_pfn + 2) >> - prep_compound_head(head, order); >> } >> + prep_compound_head(head, order); >> } >> >> void __ref memmap_init_zone_device(struct zone *zone, > memmap_init_compound() is only called in pmem case. The idea to make this /right after/ we initialize the offending tail pages has to do with @altmap case wheere struct pages are placed in PMEM and thus taking advantage of the likelyhood of those tail struct pages being cached given that we will read them right after in prep_compound_head(). I agree with the general sentiment of making this 'more resilient' to compound pages structure changes by moving prep_compound_head() after all tail pages are initialized, although I need to express a concern about this making altmap possibly being affected or regressed. Considering on 2M compound pages we will access first and second tail pages /after/ initializing 32768 struct pages, or after touching/initializing 256K struct pages.
On 2022/6/8 3:17, Joao Martins wrote: > On 6/7/22 19:32, Andrew Morton wrote: >> >> Let's cc Joao. >> >> On Tue, 7 Jun 2022 22:41:57 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: >> >>> compound_pincount_ptr is stored at first tail page instead of second tail >>> page now. >> >> "now"? Some identifiable commit did this? >> > > I think this was in: > > commit5232c63f46fd ("mm: Make compound_pincount always available") Thanks for identifying it. > >>> And if it or some other field changes again in the future, data >>> overwritten might happen. Calling prep_compound_head() outside the loop >>> to prevent such possible issue. No functional change intended. >>> >>> ... >>> >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head, >>> __init_zone_device_page(page, pfn, zone_idx, nid, pgmap); >>> prep_compound_tail(head, pfn - head_pfn); >>> set_page_count(page, 0); >>> - >>> - /* >>> - * The first tail page stores compound_mapcount_ptr() and >>> - * compound_order() and the second tail page stores >>> - * compound_pincount_ptr(). Call prep_compound_head() after >>> - * the first and second tail pages have been initialized to >>> - * not have the data overwritten. >>> - */ >>> - if (pfn == head_pfn + 2) >>> - prep_compound_head(head, order); >>> } >>> + prep_compound_head(head, order); >>> } >>> >>> void __ref memmap_init_zone_device(struct zone *zone, >> > > memmap_init_compound() is only called in pmem case. > > The idea to make this /right after/ we initialize the offending tail pages has > to do with @altmap case wheere struct pages are placed in PMEM and thus taking > advantage of the likelyhood of those tail struct pages being cached given that > we will read them right after in prep_compound_head(). > > I agree with the general sentiment of making this 'more resilient' to compound > pages structure changes by moving prep_compound_head() after all tail pages are > initialized, although I need to express a concern about this making altmap possibly > being affected or regressed. Considering on 2M compound pages we will access first and > second tail pages /after/ initializing 32768 struct pages, or after touching/initializing > 256K struct pages. Many thanks for your explanation. IIUC, the below change should be preferred? diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 4c7d99ee58b4..048df5d78add 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6771,13 +6771,18 @@ static void __ref memmap_init_compound(struct page *head, set_page_count(page, 0); /* - * The first tail page stores compound_mapcount_ptr() and - * compound_order() and the second tail page stores - * compound_pincount_ptr(). Call prep_compound_head() after - * the first and second tail pages have been initialized to - * not have the data overwritten. + * The first tail page stores compound_mapcount_ptr(), + * compound_order() and compound_pincount_ptr(). Call + * prep_compound_head() after the first tail page have + * been initialized to not have the data overwritten. + * + * Note the idea to make this right after we initialize + * the offending tail pages is trying to take advantage + * of the likelihood of those tail struct pages being + * cached given that we will read them right after in + * prep_compound_head(). */ - if (pfn == head_pfn + 2) + if (unlikely(pfn == head_pfn + 1)) prep_compound_head(head, order); } } Or am I miss something? Thanks! > . >
On Wed, Jun 08, 2022 at 08:17:35PM +0800, Miaohe Lin wrote: > +++ b/mm/page_alloc.c > @@ -6771,13 +6771,18 @@ static void __ref memmap_init_compound(struct page *head, > set_page_count(page, 0); > > /* > - * The first tail page stores compound_mapcount_ptr() and > - * compound_order() and the second tail page stores > - * compound_pincount_ptr(). Call prep_compound_head() after > - * the first and second tail pages have been initialized to > - * not have the data overwritten. > + * The first tail page stores compound_mapcount_ptr(), > + * compound_order() and compound_pincount_ptr(). Call > + * prep_compound_head() after the first tail page have > + * been initialized to not have the data overwritten. > + * > + * Note the idea to make this right after we initialize > + * the offending tail pages is trying to take advantage > + * of the likelihood of those tail struct pages being > + * cached given that we will read them right after in > + * prep_compound_head(). It's not that we'll read them again, it's that the cacheline will still be in cache, and therefore dirty. Honestly, I don't think we need this extra explanation in a comment. Just change the first paragraph to reflect reality and leave it at that. > */ > - if (pfn == head_pfn + 2) > + if (unlikely(pfn == head_pfn + 1)) We definitely don't need the unlikely here. > prep_compound_head(head, order); > } > } > > Or am I miss something? > > Thanks! > > > . > > >
On 2022/6/14 21:13, Matthew Wilcox wrote: > On Wed, Jun 08, 2022 at 08:17:35PM +0800, Miaohe Lin wrote: >> +++ b/mm/page_alloc.c >> @@ -6771,13 +6771,18 @@ static void __ref memmap_init_compound(struct page *head, >> set_page_count(page, 0); >> >> /* >> - * The first tail page stores compound_mapcount_ptr() and >> - * compound_order() and the second tail page stores >> - * compound_pincount_ptr(). Call prep_compound_head() after >> - * the first and second tail pages have been initialized to >> - * not have the data overwritten. >> + * The first tail page stores compound_mapcount_ptr(), >> + * compound_order() and compound_pincount_ptr(). Call >> + * prep_compound_head() after the first tail page have >> + * been initialized to not have the data overwritten. >> + * >> + * Note the idea to make this right after we initialize >> + * the offending tail pages is trying to take advantage >> + * of the likelihood of those tail struct pages being >> + * cached given that we will read them right after in >> + * prep_compound_head(). > > It's not that we'll read them again, it's that the cacheline will still > be in cache, and therefore dirty. Thanks for pointing this out. > > Honestly, I don't think we need this extra explanation in a comment. > Just change the first paragraph to reflect reality and leave it at that. Will do it in next version if prep_compound_head is not moved outside loop. > >> */ >> - if (pfn == head_pfn + 2) >> + if (unlikely(pfn == head_pfn + 1)) > > We definitely don't need the unlikely here. Could you please give me a more detailed explanation? IIUC, the above if condition will only meet at a probability of 1/512. So unlikely tells the compiler to do some optimization around it. Or am I miss something? Thanks! > >> prep_compound_head(head, order); >> } >> } >> >> Or am I miss something? >> >> Thanks! >> >>> . >>> >> > > . >
On Wed, Jun 15, 2022 at 03:44:06PM +0800, Miaohe Lin wrote: > > We definitely don't need the unlikely here. > > Could you please give me a more detailed explanation? IIUC, the above if condition > will only meet at a probability of 1/512. So unlikely tells the compiler to do some > optimization around it. Or am I miss something? Only add unlikely() when the compiler can't figure out for itself that it's unlikely. You should also check the generated code and/or benchmark the results to be sure that it's actually an improvement. Using unlikely() needs to be backed up with more than just a feeling.
On 2022/6/15 20:42, Matthew Wilcox wrote: > On Wed, Jun 15, 2022 at 03:44:06PM +0800, Miaohe Lin wrote: >>> We definitely don't need the unlikely here. >> >> Could you please give me a more detailed explanation? IIUC, the above if condition >> will only meet at a probability of 1/512. So unlikely tells the compiler to do some >> optimization around it. Or am I miss something? > > Only add unlikely() when the compiler can't figure out for itself that > it's unlikely. You should also check the generated code and/or > benchmark the results to be sure that it's actually an improvement. > Using unlikely() needs to be backed up with more than just a feeling. I see. Many thanks for clarifying. Will keep it in mind. :) > > . >
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 0243a21e69b1..fc3770f28d1a 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head, __init_zone_device_page(page, pfn, zone_idx, nid, pgmap); prep_compound_tail(head, pfn - head_pfn); set_page_count(page, 0); - - /* - * The first tail page stores compound_mapcount_ptr() and - * compound_order() and the second tail page stores - * compound_pincount_ptr(). Call prep_compound_head() after - * the first and second tail pages have been initialized to - * not have the data overwritten. - */ - if (pfn == head_pfn + 2) - prep_compound_head(head, order); } + prep_compound_head(head, order); } void __ref memmap_init_zone_device(struct zone *zone,
compound_pincount_ptr is stored at first tail page instead of second tail page now. And if it or some other field changes again in the future, data overwritten might happen. Calling prep_compound_head() outside the loop to prevent such possible issue. No functional change intended. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/page_alloc.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)