| Message ID | 20220304093409.25829-14-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for migration | expand |
On Fri, Mar 4, 2022 at 5:36 PM Miaohe Lin <linmiaohe@huawei.com> wrote: > > We should return errno (-EBUSY here) when failed to isolate the huge page > rather than always return 1 which could confuse the user. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> IIUC, this should be a bug fix since the status reported to the user is wrong. A Fixes tag may be needed. But anyway: Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
Hi Miaohe, On 3/4/2022 5:34 PM, Miaohe Lin wrote: > We should return errno (-EBUSY here) when failed to isolate the huge page > rather than always return 1 which could confuse the user. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/migrate.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 6c2dfed2ddb8..279940c0c064 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, > goto out_putpage; > > if (PageHuge(page)) { > - if (PageHead(page)) { > - isolate_huge_page(page, pagelist); > - err = 1; > - } > + if (PageHead(page)) > + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; Could you elaborate on which case the huge page isolation can be failed in this case? Or you met a real problem? Cause now we've got this head huge page refcnt, I can not see why we'll fail to isolate this huge page.
Miaohe Lin <linmiaohe@huawei.com> writes: > We should return errno (-EBUSY here) when failed to isolate the huge page > rather than always return 1 which could confuse the user. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/migrate.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 6c2dfed2ddb8..279940c0c064 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, > goto out_putpage; > > if (PageHuge(page)) { > - if (PageHead(page)) { > - isolate_huge_page(page, pagelist); > - err = 1; > - } > + if (PageHead(page)) > + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; IMHO, it's better to determine the proper errno inside isolate_huge_page() instead of in the caller. If you think it's necessary to get errno here. How about change isolate_huge_page() instead? Best Regards, Huang, Ying > } else { > struct page *head;
On 2022/3/5 10:23, Muchun Song wrote: > On Fri, Mar 4, 2022 at 5:36 PM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> We should return errno (-EBUSY here) when failed to isolate the huge page >> rather than always return 1 which could confuse the user. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > IIUC, this should be a bug fix since the status reported to the user is > wrong. A Fixes tag may be needed. But anyway: > Agree. Thanks for pointing this out and many thanks for your review. :) > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > > Thanks. > > . >
On 2022/3/7 10:14, Baolin Wang wrote: > Hi Miaohe, > > On 3/4/2022 5:34 PM, Miaohe Lin wrote: >> We should return errno (-EBUSY here) when failed to isolate the huge page >> rather than always return 1 which could confuse the user. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/migrate.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 6c2dfed2ddb8..279940c0c064 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >> goto out_putpage; >> if (PageHuge(page)) { >> - if (PageHead(page)) { >> - isolate_huge_page(page, pagelist); >> - err = 1; >> - } >> + if (PageHead(page)) >> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; > > Could you elaborate on which case the huge page isolation can be failed in this case? Or you met a real problem? Cause now we've got this head huge page refcnt, I can not see why we'll fail to isolate this huge page. IIUC, this could happen when hugepage is under migration which cleared HPageMigratable. Page refcnt cannot prevent isolate_huge_page from happening. Or am I miss something? Many thanks. > .
On 3/7/2022 8:20 PM, Miaohe Lin wrote: > On 2022/3/7 10:14, Baolin Wang wrote: >> Hi Miaohe, >> >> On 3/4/2022 5:34 PM, Miaohe Lin wrote: >>> We should return errno (-EBUSY here) when failed to isolate the huge page >>> rather than always return 1 which could confuse the user. >>> >>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>> --- >>> mm/migrate.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 6c2dfed2ddb8..279940c0c064 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >>> goto out_putpage; >>> if (PageHuge(page)) { >>> - if (PageHead(page)) { >>> - isolate_huge_page(page, pagelist); >>> - err = 1; >>> - } >>> + if (PageHead(page)) >>> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; >> >> Could you elaborate on which case the huge page isolation can be failed in this case? Or you met a real problem? Cause now we've got this head huge page refcnt, I can not see why we'll fail to isolate this huge page. > > IIUC, this could happen when hugepage is under migration which cleared HPageMigratable. Page refcnt cannot > prevent isolate_huge_page from happening. Or am I miss something? Yes, that's possible. Thanks for your explanation. It will be better if you can copy the possible scenario description to the commit log to help other understand the issue. Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
On 2022/3/8 9:32, Baolin Wang wrote: > > > On 3/7/2022 8:20 PM, Miaohe Lin wrote: >> On 2022/3/7 10:14, Baolin Wang wrote: >>> Hi Miaohe, >>> >>> On 3/4/2022 5:34 PM, Miaohe Lin wrote: >>>> We should return errno (-EBUSY here) when failed to isolate the huge page >>>> rather than always return 1 which could confuse the user. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/migrate.c | 6 ++---- >>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/mm/migrate.c b/mm/migrate.c >>>> index 6c2dfed2ddb8..279940c0c064 100644 >>>> --- a/mm/migrate.c >>>> +++ b/mm/migrate.c >>>> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >>>> goto out_putpage; >>>> if (PageHuge(page)) { >>>> - if (PageHead(page)) { >>>> - isolate_huge_page(page, pagelist); >>>> - err = 1; >>>> - } >>>> + if (PageHead(page)) >>>> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; >>> >>> Could you elaborate on which case the huge page isolation can be failed in this case? Or you met a real problem? Cause now we've got this head huge page refcnt, I can not see why we'll fail to isolate this huge page. >> >> IIUC, this could happen when hugepage is under migration which cleared HPageMigratable. Page refcnt cannot >> prevent isolate_huge_page from happening. Or am I miss something? > > Yes, that's possible. Thanks for your explanation. It will be better if you can copy the possible scenario description to the commit log to help other understand the issue. > Sounds reasonable. Will do. Many thanks for review. > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> > .
On 2022/3/7 13:07, Huang, Ying wrote: > Miaohe Lin <linmiaohe@huawei.com> writes: > >> We should return errno (-EBUSY here) when failed to isolate the huge page >> rather than always return 1 which could confuse the user. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/migrate.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 6c2dfed2ddb8..279940c0c064 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >> goto out_putpage; >> >> if (PageHuge(page)) { >> - if (PageHead(page)) { >> - isolate_huge_page(page, pagelist); >> - err = 1; >> - } >> + if (PageHead(page)) >> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; > > IMHO, it's better to determine the proper errno inside > isolate_huge_page() instead of in the caller. If you think it's > necessary to get errno here. How about change isolate_huge_page() > instead? IMO, -EBUSY should be enough for the user (as they could not do much) and this errno keeps consistent with the non-hugetlb page case. What do you think? Thanks. > > Best Regards, > Huang, Ying > >> } else { >> struct page *head; > . >
Miaohe Lin <linmiaohe@huawei.com> writes: > On 2022/3/7 13:07, Huang, Ying wrote: >> Miaohe Lin <linmiaohe@huawei.com> writes: >> >>> We should return errno (-EBUSY here) when failed to isolate the huge page >>> rather than always return 1 which could confuse the user. >>> >>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>> --- >>> mm/migrate.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 6c2dfed2ddb8..279940c0c064 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >>> goto out_putpage; >>> >>> if (PageHuge(page)) { >>> - if (PageHead(page)) { >>> - isolate_huge_page(page, pagelist); >>> - err = 1; >>> - } >>> + if (PageHead(page)) >>> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; >> >> IMHO, it's better to determine the proper errno inside >> isolate_huge_page() instead of in the caller. If you think it's >> necessary to get errno here. How about change isolate_huge_page() >> instead? > > IMO, -EBUSY should be enough for the user (as they could not do much) and this > errno keeps consistent with the non-hugetlb page case. What do you think? I found the prototype of isolate_lru_page() is as follows, int isolate_lru_page(struct page *page) And it will return errno directly. I think we should follow same convention here? Best Regards, Huang, Ying
On 2022/3/9 9:00, Huang, Ying wrote: > Miaohe Lin <linmiaohe@huawei.com> writes: > >> On 2022/3/7 13:07, Huang, Ying wrote: >>> Miaohe Lin <linmiaohe@huawei.com> writes: >>> >>>> We should return errno (-EBUSY here) when failed to isolate the huge page >>>> rather than always return 1 which could confuse the user. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/migrate.c | 6 ++---- >>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/mm/migrate.c b/mm/migrate.c >>>> index 6c2dfed2ddb8..279940c0c064 100644 >>>> --- a/mm/migrate.c >>>> +++ b/mm/migrate.c >>>> @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, >>>> goto out_putpage; >>>> >>>> if (PageHuge(page)) { >>>> - if (PageHead(page)) { >>>> - isolate_huge_page(page, pagelist); >>>> - err = 1; >>>> - } >>>> + if (PageHead(page)) >>>> + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; >>> >>> IMHO, it's better to determine the proper errno inside >>> isolate_huge_page() instead of in the caller. If you think it's >>> necessary to get errno here. How about change isolate_huge_page() >>> instead? >> >> IMO, -EBUSY should be enough for the user (as they could not do much) and this >> errno keeps consistent with the non-hugetlb page case. What do you think? > > I found the prototype of isolate_lru_page() is as follows, > > int isolate_lru_page(struct page *page) > > And it will return errno directly. I think we should follow same > convention here? > I see. Sounds reasonable to me. Will try to do it. Thanks. > Best Regards, > Huang, Ying > . >
diff --git a/mm/migrate.c b/mm/migrate.c index 6c2dfed2ddb8..279940c0c064 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1618,10 +1618,8 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, goto out_putpage; if (PageHuge(page)) { - if (PageHead(page)) { - isolate_huge_page(page, pagelist); - err = 1; - } + if (PageHead(page)) + err = isolate_huge_page(page, pagelist) ? 1 : -EBUSY; } else { struct page *head;
We should return errno (-EBUSY here) when failed to isolate the huge page rather than always return 1 which could confuse the user. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/migrate.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)