| Message ID | 20220816130553.31406-2-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few fixup patches for hugetlb | expand |
On 08/16/22 21:05, Miaohe Lin wrote: > There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) > pages incremented for target_hstate->max_huge_pages when page is demoted. > Update max_huge_pages accordingly for consistency. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/hugetlb.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index ea1c7bfa1cc3..e72052964fb5 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3472,7 +3472,8 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) > * based on pool changes for the demoted page. > */ > h->max_huge_pages--; > - target_hstate->max_huge_pages += pages_per_huge_page(h); > + target_hstate->max_huge_pages += > + pages_per_huge_page(h) / pages_per_huge_page(target_hstate); Thanks! That is indeed incorrect. However the miscalculation should not have any consequences. Correct? The value is used when initially populating the pools. It is never read and used again. It is written to in set_max_huge_pages if someone changes the number of hugetlb pages. I guess that is a long way of saying I am not sure why we care about trying to keep max_huge_pages up to date? I do not think it matters. I also thought, if we are going to adjust max_huge_pages here we may also want to adjust the node specific value: h->max_huge_pages_node[node]. There are a few other places where the global max_huge_pages is adjusted without adjusting the node specific value. The more I think about it, the more I think we should explore just eliminating any adjustment of this/these values after initially populating the pools.
On Tue, 16 Aug 2022 15:52:47 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote: > On 08/16/22 21:05, Miaohe Lin wrote: > > There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) > > pages incremented for target_hstate->max_huge_pages when page is demoted. > > Update max_huge_pages accordingly for consistency. > > > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > --- > > mm/hugetlb.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index ea1c7bfa1cc3..e72052964fb5 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -3472,7 +3472,8 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) > > * based on pool changes for the demoted page. > > */ > > h->max_huge_pages--; > > - target_hstate->max_huge_pages += pages_per_huge_page(h); > > + target_hstate->max_huge_pages += > > + pages_per_huge_page(h) / pages_per_huge_page(target_hstate); > > Thanks! > > That is indeed incorrect. However the miscalculation should not have any > consequences. Correct? The value is used when initially populating the > pools. It is never read and used again. It is written to in > set_max_huge_pages if someone changes the number of hugetlb pages. > > I guess that is a long way of saying I am not sure why we care about trying > to keep max_huge_pages up to date? I do not think it matters. > > I also thought, if we are going to adjust max_huge_pages here we may > also want to adjust the node specific value: h->max_huge_pages_node[node]. > There are a few other places where the global max_huge_pages is adjusted > without adjusting the node specific value. > > The more I think about it, the more I think we should explore just > eliminating any adjustment of this/these values after initially > populating the pools. I'm thinking we should fix something that is "indeed incorrect" before going on to more extensive things?
On 08/16/22 16:20, Andrew Morton wrote: > On Tue, 16 Aug 2022 15:52:47 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote: > > > On 08/16/22 21:05, Miaohe Lin wrote: > > > There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) > > > pages incremented for target_hstate->max_huge_pages when page is demoted. > > > Update max_huge_pages accordingly for consistency. > > > > > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > > --- > > > mm/hugetlb.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > index ea1c7bfa1cc3..e72052964fb5 100644 > > > --- a/mm/hugetlb.c > > > +++ b/mm/hugetlb.c > > > @@ -3472,7 +3472,8 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) > > > * based on pool changes for the demoted page. > > > */ > > > h->max_huge_pages--; > > > - target_hstate->max_huge_pages += pages_per_huge_page(h); > > > + target_hstate->max_huge_pages += > > > + pages_per_huge_page(h) / pages_per_huge_page(target_hstate); > > > > Thanks! > > > > That is indeed incorrect. However the miscalculation should not have any > > consequences. Correct? The value is used when initially populating the > > pools. It is never read and used again. It is written to in > > set_max_huge_pages if someone changes the number of hugetlb pages. > > > > I guess that is a long way of saying I am not sure why we care about trying > > to keep max_huge_pages up to date? I do not think it matters. > > > > I also thought, if we are going to adjust max_huge_pages here we may > > also want to adjust the node specific value: h->max_huge_pages_node[node]. > > There are a few other places where the global max_huge_pages is adjusted > > without adjusting the node specific value. > > > > The more I think about it, the more I think we should explore just > > eliminating any adjustment of this/these values after initially > > populating the pools. > > I'm thinking we should fix something that is "indeed incorrect" before > going on to more extensive things? Sure, I am good with that. Just wanted to point out that the incorrect calculation does not have any negative consequences. Maybe prompting Miaohe to look into the more extensive cleanup.
On 2022/8/17 7:34, Mike Kravetz wrote: > On 08/16/22 16:20, Andrew Morton wrote: >> On Tue, 16 Aug 2022 15:52:47 -0700 Mike Kravetz <mike.kravetz@oracle.com> wrote: >> >>> On 08/16/22 21:05, Miaohe Lin wrote: >>>> There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) >>>> pages incremented for target_hstate->max_huge_pages when page is demoted. >>>> Update max_huge_pages accordingly for consistency. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/hugetlb.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>> index ea1c7bfa1cc3..e72052964fb5 100644 >>>> --- a/mm/hugetlb.c >>>> +++ b/mm/hugetlb.c >>>> @@ -3472,7 +3472,8 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) >>>> * based on pool changes for the demoted page. >>>> */ >>>> h->max_huge_pages--; >>>> - target_hstate->max_huge_pages += pages_per_huge_page(h); >>>> + target_hstate->max_huge_pages += >>>> + pages_per_huge_page(h) / pages_per_huge_page(target_hstate); >>> >>> Thanks! >>> >>> That is indeed incorrect. However the miscalculation should not have any >>> consequences. Correct? The value is used when initially populating the >>> pools. It is never read and used again. It is written to in >>> set_max_huge_pages if someone changes the number of hugetlb pages. >>> >>> I guess that is a long way of saying I am not sure why we care about trying >>> to keep max_huge_pages up to date? I do not think it matters. >>> >>> I also thought, if we are going to adjust max_huge_pages here we may >>> also want to adjust the node specific value: h->max_huge_pages_node[node]. >>> There are a few other places where the global max_huge_pages is adjusted >>> without adjusting the node specific value. >>> >>> The more I think about it, the more I think we should explore just >>> eliminating any adjustment of this/these values after initially >>> populating the pools. >> >> I'm thinking we should fix something that is "indeed incorrect" before >> going on to more extensive things? > > Sure, I am good with that. > > Just wanted to point out that the incorrect calculation does not have > any negative consequences. Maybe prompting Miaohe to look into the more > extensive cleanup. Many thanks both. I will try to do this "more extensive cleanup" after pending work is done. Thanks, Miaohe Lin
> On Aug 16, 2022, at 21:05, Miaohe Lin <linmiaohe@huawei.com> wrote: > > There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) > pages incremented for target_hstate->max_huge_pages when page is demoted. > Update max_huge_pages accordingly for consistency. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index ea1c7bfa1cc3..e72052964fb5 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3472,7 +3472,8 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) * based on pool changes for the demoted page. */ h->max_huge_pages--; - target_hstate->max_huge_pages += pages_per_huge_page(h); + target_hstate->max_huge_pages += + pages_per_huge_page(h) / pages_per_huge_page(target_hstate); return rc; }
There should be pages_per_huge_page(h) / pages_per_huge_page(target_hstate) pages incremented for target_hstate->max_huge_pages when page is demoted. Update max_huge_pages accordingly for consistency. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/hugetlb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)