| Message ID | 20210402093249.25137-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup and fixup for hugetlb | expand |
On 4/2/21 2:32 AM, Miaohe Lin wrote: > A rare out of memory error would prevent removal of the reserve map region > for a page. hugetlb_fix_reserve_counts() handles this rare case to avoid > dangling with incorrect counts. Unfortunately, hugepage_subpool_get_pages > and hugetlb_acct_memory could possibly fail too. We should correctly handle > these cases. Yes, this is a potential issue. The 'good news' is that hugetlb_fix_reserve_counts() is unlikely to ever be called. To do so would imply we could not allocate a region entry which is only 6 words in size. We also keep a 'cache' of entries so we may not even need to allocate. But, as mentioned it is a potential issue. > Fixes: b5cec28d36f5 ("hugetlbfs: truncate_hugepages() takes a range of pages") This is likely going to make this get picked by by stable releases. That is unfortunate as mentioned above this is mostly theoretical. > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/hugetlb.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index bdff8d23803f..ca5464ed04b7 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -745,13 +745,20 @@ void hugetlb_fix_reserve_counts(struct inode *inode) > { > struct hugepage_subpool *spool = subpool_inode(inode); > long rsv_adjust; > + bool reserved = false; > > rsv_adjust = hugepage_subpool_get_pages(spool, 1); > - if (rsv_adjust) { > + if (rsv_adjust > 0) { > struct hstate *h = hstate_inode(inode); > > - hugetlb_acct_memory(h, 1); > + if (!hugetlb_acct_memory(h, 1)) > + reserved = true; > + } else if (!rsv_adjust) { > + reserved = true; > } > + > + if (!reserved) > + pr_warn("hugetlb: fix reserve count failed\n"); We should expand this warning message a bit to indicate what this may mean to the user. Add something like" "Huge Page Reserved count may go negative".
On 2021/4/9 7:25, Mike Kravetz wrote: > On 4/2/21 2:32 AM, Miaohe Lin wrote: >> A rare out of memory error would prevent removal of the reserve map region >> for a page. hugetlb_fix_reserve_counts() handles this rare case to avoid >> dangling with incorrect counts. Unfortunately, hugepage_subpool_get_pages >> and hugetlb_acct_memory could possibly fail too. We should correctly handle >> these cases. > > Yes, this is a potential issue. > > The 'good news' is that hugetlb_fix_reserve_counts() is unlikely to ever > be called. To do so would imply we could not allocate a region entry > which is only 6 words in size. We also keep a 'cache' of entries so we > may not even need to allocate. > > But, as mentioned it is a potential issue. Yes, a potential *theoretical* issue. > >> Fixes: b5cec28d36f5 ("hugetlbfs: truncate_hugepages() takes a range of pages") > > This is likely going to make this get picked by by stable releases. > That is unfortunate as mentioned above this is mostly theoretical. > I will drop this. This does not worth backport. >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/hugetlb.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index bdff8d23803f..ca5464ed04b7 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -745,13 +745,20 @@ void hugetlb_fix_reserve_counts(struct inode *inode) >> { >> struct hugepage_subpool *spool = subpool_inode(inode); >> long rsv_adjust; >> + bool reserved = false; >> >> rsv_adjust = hugepage_subpool_get_pages(spool, 1); >> - if (rsv_adjust) { >> + if (rsv_adjust > 0) { >> struct hstate *h = hstate_inode(inode); >> >> - hugetlb_acct_memory(h, 1); >> + if (!hugetlb_acct_memory(h, 1)) >> + reserved = true; >> + } else if (!rsv_adjust) { >> + reserved = true; >> } >> + >> + if (!reserved) >> + pr_warn("hugetlb: fix reserve count failed\n"); > > We should expand this warning message a bit to indicate what this may > mean to the user. Add something like" > "Huge Page Reserved count may go negative". > Will add it in v2. Many thanks for review and nice suggestion ! :)
On Fri, 9 Apr 2021 11:17:49 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2021/4/9 7:25, Mike Kravetz wrote: > > On 4/2/21 2:32 AM, Miaohe Lin wrote: > >> A rare out of memory error would prevent removal of the reserve map region > >> for a page. hugetlb_fix_reserve_counts() handles this rare case to avoid > >> dangling with incorrect counts. Unfortunately, hugepage_subpool_get_pages > >> and hugetlb_acct_memory could possibly fail too. We should correctly handle > >> these cases. > > > > Yes, this is a potential issue. > > > > The 'good news' is that hugetlb_fix_reserve_counts() is unlikely to ever > > be called. To do so would imply we could not allocate a region entry > > which is only 6 words in size. We also keep a 'cache' of entries so we > > may not even need to allocate. > > > > But, as mentioned it is a potential issue. > > Yes, a potential *theoretical* issue. > > > > >> Fixes: b5cec28d36f5 ("hugetlbfs: truncate_hugepages() takes a range of pages") > > > > This is likely going to make this get picked by by stable releases. > > That is unfortunate as mentioned above this is mostly theoretical. > > > > I will drop this. This does not worth backport. > -stable have been asked not to backport MM patches unless MM patches include "cc:stable". ie, no making our backporting decisions for us, please.
On 2021/4/9 13:04, Andrew Morton wrote: > On Fri, 9 Apr 2021 11:17:49 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > >> On 2021/4/9 7:25, Mike Kravetz wrote: >>> On 4/2/21 2:32 AM, Miaohe Lin wrote: >>>> A rare out of memory error would prevent removal of the reserve map region >>>> for a page. hugetlb_fix_reserve_counts() handles this rare case to avoid >>>> dangling with incorrect counts. Unfortunately, hugepage_subpool_get_pages >>>> and hugetlb_acct_memory could possibly fail too. We should correctly handle >>>> these cases. >>> >>> Yes, this is a potential issue. >>> >>> The 'good news' is that hugetlb_fix_reserve_counts() is unlikely to ever >>> be called. To do so would imply we could not allocate a region entry >>> which is only 6 words in size. We also keep a 'cache' of entries so we >>> may not even need to allocate. >>> >>> But, as mentioned it is a potential issue. >> >> Yes, a potential *theoretical* issue. >> >>> >>>> Fixes: b5cec28d36f5 ("hugetlbfs: truncate_hugepages() takes a range of pages") >>> >>> This is likely going to make this get picked by by stable releases. >>> That is unfortunate as mentioned above this is mostly theoretical. >>> >> >> I will drop this. This does not worth backport. >> > > -stable have been asked not to backport MM patches unless MM patches > include "cc:stable". ie, no making our backporting decisions for us, > please. > Sorry about it! I would retain the Fixes tag. Many thanks for pointing this out. > . >
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index bdff8d23803f..ca5464ed04b7 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -745,13 +745,20 @@ void hugetlb_fix_reserve_counts(struct inode *inode) { struct hugepage_subpool *spool = subpool_inode(inode); long rsv_adjust; + bool reserved = false; rsv_adjust = hugepage_subpool_get_pages(spool, 1); - if (rsv_adjust) { + if (rsv_adjust > 0) { struct hstate *h = hstate_inode(inode); - hugetlb_acct_memory(h, 1); + if (!hugetlb_acct_memory(h, 1)) + reserved = true; + } else if (!rsv_adjust) { + reserved = true; } + + if (!reserved) + pr_warn("hugetlb: fix reserve count failed\n"); } /*
A rare out of memory error would prevent removal of the reserve map region for a page. hugetlb_fix_reserve_counts() handles this rare case to avoid dangling with incorrect counts. Unfortunately, hugepage_subpool_get_pages and hugetlb_acct_memory could possibly fail too. We should correctly handle these cases. Fixes: b5cec28d36f5 ("hugetlbfs: truncate_hugepages() takes a range of pages") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/hugetlb.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)