| Message ID | 20240726082818.260008-1-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: huge_memory: don't start_stop_khugepaged for non-PMD THP | expand |
On 26/07/2024 09:28, Barry Song wrote: > From: Barry Song <v-songbaohua@oppo.com> > > khugepaged will be automatically started when PMD-sized THP is enabled > (either of the per-size anon control or the top-level control are set > to "always" or "madvise"), and it'll be automatically shutdown when > PMD-sized THP is disabled (when both the per-size anon control and the > top-level control are "never"). > > It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, > as it would only waste CPU time. > > Cc: Lance Yang <ioworker0@gmail.com> > Cc: Ryan Roberts <ryan.roberts@arm.com> > Cc: Baolin Wang <baolin.wang@linux.alibaba.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: Zi Yan <ziy@nvidia.com> > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > --- > mm/huge_memory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 41460847988c..bd365e35acf7 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, > } else > ret = -EINVAL; > > - if (ret > 0) { > + if (ret > 0 && order == HPAGE_PMD_ORDER) { > int err; > > err = start_stop_khugepaged(); Personally I see this as a bit of a layering violation; its start_stop_khugepaged() that should decide the policy for when to start and stop the daemon. thpsize_enabled_store() should just be calling start_stop_khugepaged() to notify that something potentially pertinent to the a policy decision has changed. And I don't think this should be a hot path; I'd expect the global thp size configuration to be set at boot and remain pretty static. Do you have evidence to the contrary?
On 26.07.24 11:43, Ryan Roberts wrote: > On 26/07/2024 09:28, Barry Song wrote: >> From: Barry Song <v-songbaohua@oppo.com> >> >> khugepaged will be automatically started when PMD-sized THP is enabled >> (either of the per-size anon control or the top-level control are set >> to "always" or "madvise"), and it'll be automatically shutdown when >> PMD-sized THP is disabled (when both the per-size anon control and the >> top-level control are "never"). >> >> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, >> as it would only waste CPU time. >> >> Cc: Lance Yang <ioworker0@gmail.com> >> Cc: Ryan Roberts <ryan.roberts@arm.com> >> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> >> Cc: David Hildenbrand <david@redhat.com> >> Cc: Yang Shi <shy828301@gmail.com> >> Cc: Zi Yan <ziy@nvidia.com> >> Signed-off-by: Barry Song <v-songbaohua@oppo.com> >> --- >> mm/huge_memory.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 41460847988c..bd365e35acf7 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, >> } else >> ret = -EINVAL; >> >> - if (ret > 0) { >> + if (ret > 0 && order == HPAGE_PMD_ORDER) { >> int err; >> >> err = start_stop_khugepaged(); > > Personally I see this as a bit of a layering violation; its > start_stop_khugepaged() that should decide the policy for when to start and stop > the daemon. thpsize_enabled_store() should just be calling > start_stop_khugepaged() to notify that something potentially pertinent to the a > policy decision has changed. Agreed, skimming the subject I was under the impression that we would be fixing something here.
On Fri, Jul 26, 2024 at 9:48 PM David Hildenbrand <david@redhat.com> wrote: > > On 26.07.24 11:43, Ryan Roberts wrote: > > On 26/07/2024 09:28, Barry Song wrote: > >> From: Barry Song <v-songbaohua@oppo.com> > >> > >> khugepaged will be automatically started when PMD-sized THP is enabled > >> (either of the per-size anon control or the top-level control are set > >> to "always" or "madvise"), and it'll be automatically shutdown when > >> PMD-sized THP is disabled (when both the per-size anon control and the > >> top-level control are "never"). > >> > >> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, > >> as it would only waste CPU time. > >> > >> Cc: Lance Yang <ioworker0@gmail.com> > >> Cc: Ryan Roberts <ryan.roberts@arm.com> > >> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> > >> Cc: David Hildenbrand <david@redhat.com> > >> Cc: Yang Shi <shy828301@gmail.com> > >> Cc: Zi Yan <ziy@nvidia.com> > >> Signed-off-by: Barry Song <v-songbaohua@oppo.com> > >> --- > >> mm/huge_memory.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index 41460847988c..bd365e35acf7 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, > >> } else > >> ret = -EINVAL; > >> > >> - if (ret > 0) { > >> + if (ret > 0 && order == HPAGE_PMD_ORDER) { > >> int err; > >> > >> err = start_stop_khugepaged(); > > > > Personally I see this as a bit of a layering violation; its > > start_stop_khugepaged() that should decide the policy for when to start and stop > > the daemon. thpsize_enabled_store() should just be calling > > start_stop_khugepaged() to notify that something potentially pertinent to the a > > policy decision has changed. > My impression is that it slightly deviates from the huge page documentation in Documentation/admin-guide/mm/transhuge.rst. khugepaged will be automatically started when PMD-sized THP is enabled (either of the per-size anon control or the top-level control are set to "always" or "madvise"), and it'll be automatically shutdown when PMD-sized THP is disabled (when both the per-size anon control and the top-level control are "never"). non-PMD size is not involved in khugepaged, but I agree the policy might change in the future. > Agreed, skimming the subject I was under the impression that we would be > fixing something here. working on another swapin_enabled and reviewing the enabled source code. I don't need this startstop for all sizes in that case, so I made a quick adjustment to this part as well. If neither of you likes it, that's fine with me :-) > > -- > Cheers, > > David / dhildenb > Thanks Barry
On Fri, Jul 26, 2024 at 9:43 PM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 26/07/2024 09:28, Barry Song wrote: > > From: Barry Song <v-songbaohua@oppo.com> > > > > khugepaged will be automatically started when PMD-sized THP is enabled > > (either of the per-size anon control or the top-level control are set > > to "always" or "madvise"), and it'll be automatically shutdown when > > PMD-sized THP is disabled (when both the per-size anon control and the > > top-level control are "never"). > > > > It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, > > as it would only waste CPU time. > > > > Cc: Lance Yang <ioworker0@gmail.com> > > Cc: Ryan Roberts <ryan.roberts@arm.com> > > Cc: Baolin Wang <baolin.wang@linux.alibaba.com> > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Yang Shi <shy828301@gmail.com> > > Cc: Zi Yan <ziy@nvidia.com> > > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > > --- > > mm/huge_memory.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 41460847988c..bd365e35acf7 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, > > } else > > ret = -EINVAL; > > > > - if (ret > 0) { > > + if (ret > 0 && order == HPAGE_PMD_ORDER) { > > int err; > > > > err = start_stop_khugepaged(); > > Personally I see this as a bit of a layering violation; its > start_stop_khugepaged() that should decide the policy for when to start and stop > the daemon. thpsize_enabled_store() should just be calling > start_stop_khugepaged() to notify that something potentially pertinent to the a > policy decision has changed. > > And I don't think this should be a hot path; I'd expect the global thp size > configuration to be set at boot and remain pretty static. Do you have evidence > to the contrary? Absolutely no, I was reading and patching the doc "Documentation/admin-guide/mm/transhuge.rst" and was also copying-paste your "enabled" code to "swapin_enabled" with dropping start_stop_khugepaged() in that case. I believe I don't need it for enabling/disabling swapin as swapin is a subset of mTHP not a main entry. just feel a bit inconsistency between the doc and the code at least for this moment: khugepaged will be automatically started when PMD-sized THP is enabled (either of the per-size anon control or the top-level control are set to "always" or "madvise"), and it'll be automatically shutdown when PMD-sized THP is disabled (when both the per-size anon control and the top-level control are "never"). which triggered me to make this change. Thanks Barry
On 26/07/2024 11:04, Barry Song wrote: > On Fri, Jul 26, 2024 at 9:48 PM David Hildenbrand <david@redhat.com> wrote: >> >> On 26.07.24 11:43, Ryan Roberts wrote: >>> On 26/07/2024 09:28, Barry Song wrote: >>>> From: Barry Song <v-songbaohua@oppo.com> >>>> >>>> khugepaged will be automatically started when PMD-sized THP is enabled >>>> (either of the per-size anon control or the top-level control are set >>>> to "always" or "madvise"), and it'll be automatically shutdown when >>>> PMD-sized THP is disabled (when both the per-size anon control and the >>>> top-level control are "never"). >>>> >>>> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, >>>> as it would only waste CPU time. >>>> >>>> Cc: Lance Yang <ioworker0@gmail.com> >>>> Cc: Ryan Roberts <ryan.roberts@arm.com> >>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Cc: Yang Shi <shy828301@gmail.com> >>>> Cc: Zi Yan <ziy@nvidia.com> >>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com> >>>> --- >>>> mm/huge_memory.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index 41460847988c..bd365e35acf7 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, >>>> } else >>>> ret = -EINVAL; >>>> >>>> - if (ret > 0) { >>>> + if (ret > 0 && order == HPAGE_PMD_ORDER) { >>>> int err; >>>> >>>> err = start_stop_khugepaged(); >>> >>> Personally I see this as a bit of a layering violation; its >>> start_stop_khugepaged() that should decide the policy for when to start and stop >>> the daemon. thpsize_enabled_store() should just be calling >>> start_stop_khugepaged() to notify that something potentially pertinent to the a >>> policy decision has changed. >> > > My impression is that it slightly deviates from the huge page documentation in > Documentation/admin-guide/mm/transhuge.rst. > > khugepaged will be automatically started when PMD-sized THP is enabled > (either of the per-size anon control or the top-level control are set > to "always" or "madvise"), and it'll be automatically shutdown when > PMD-sized THP is disabled (when both the per-size anon control and the > top-level control are "never"). But start_stop_khugepaged() doesn't unconditionally start khugepaged, it takes action based on hugepage_pmd_enabled() which only returns true if there are any pmd sized THP enabled (currently looking at anon and file, but should also look at shmem in future; that's a known bug that's been there forever). So I don't think it is inconsistent with the documentation? > > non-PMD size is not involved in khugepaged, but I agree the policy might change > in the future. > >> Agreed, skimming the subject I was under the impression that we would be >> fixing something here. > > working on another swapin_enabled and reviewing the enabled source code. > I don't need this startstop for all sizes in that case, so I made a > quick adjustment > to this part as well. If neither of you likes it, that's fine with me :-) > >> >> -- >> Cheers, >> >> David / dhildenb >> > > Thanks > Barry
On Fri, Jul 26, 2024 at 10:45 PM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 26/07/2024 11:04, Barry Song wrote: > > On Fri, Jul 26, 2024 at 9:48 PM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 26.07.24 11:43, Ryan Roberts wrote: > >>> On 26/07/2024 09:28, Barry Song wrote: > >>>> From: Barry Song <v-songbaohua@oppo.com> > >>>> > >>>> khugepaged will be automatically started when PMD-sized THP is enabled > >>>> (either of the per-size anon control or the top-level control are set > >>>> to "always" or "madvise"), and it'll be automatically shutdown when > >>>> PMD-sized THP is disabled (when both the per-size anon control and the > >>>> top-level control are "never"). > >>>> > >>>> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, > >>>> as it would only waste CPU time. > >>>> > >>>> Cc: Lance Yang <ioworker0@gmail.com> > >>>> Cc: Ryan Roberts <ryan.roberts@arm.com> > >>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> > >>>> Cc: David Hildenbrand <david@redhat.com> > >>>> Cc: Yang Shi <shy828301@gmail.com> > >>>> Cc: Zi Yan <ziy@nvidia.com> > >>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com> > >>>> --- > >>>> mm/huge_memory.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >>>> index 41460847988c..bd365e35acf7 100644 > >>>> --- a/mm/huge_memory.c > >>>> +++ b/mm/huge_memory.c > >>>> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, > >>>> } else > >>>> ret = -EINVAL; > >>>> > >>>> - if (ret > 0) { > >>>> + if (ret > 0 && order == HPAGE_PMD_ORDER) { > >>>> int err; > >>>> > >>>> err = start_stop_khugepaged(); > >>> > >>> Personally I see this as a bit of a layering violation; its > >>> start_stop_khugepaged() that should decide the policy for when to start and stop > >>> the daemon. thpsize_enabled_store() should just be calling > >>> start_stop_khugepaged() to notify that something potentially pertinent to the a > >>> policy decision has changed. > >> > > > > My impression is that it slightly deviates from the huge page documentation in > > Documentation/admin-guide/mm/transhuge.rst. > > > > khugepaged will be automatically started when PMD-sized THP is enabled > > (either of the per-size anon control or the top-level control are set > > to "always" or "madvise"), and it'll be automatically shutdown when > > PMD-sized THP is disabled (when both the per-size anon control and the > > top-level control are "never"). > > But start_stop_khugepaged() doesn't unconditionally start khugepaged, it takes > action based on hugepage_pmd_enabled() which only returns true if there are any > pmd sized THP enabled (currently looking at anon and file, but should also look > at shmem in future; that's a known bug that's been there forever). So I don't > think it is inconsistent with the documentation? My point is that, as a code reader, if non-PMD sizes are never involved with khugepaged, we can proceed without needing to check the lower-layer code. otherwise it is a bit confusing, especially since start_stop_khugepaged() unconditionally calls set_recommended_min_free_kbytes() for all sizes, regardless of whether they are PMD. However, I agree with your point that if non-PMD sizes are eventually managed by khugepaged, then this approach is flawed. This is the downside of this change. > > > > > non-PMD size is not involved in khugepaged, but I agree the policy might change > > in the future. > > > >> Agreed, skimming the subject I was under the impression that we would be > >> fixing something here. > > > > working on another swapin_enabled and reviewing the enabled source code. > > I don't need this startstop for all sizes in that case, so I made a > > quick adjustment > > to this part as well. If neither of you likes it, that's fine with me :-) > > > >> > >> -- > >> Cheers, > >> > >> David / dhildenb > >> Thanks Barry
On 26/07/2024 12:00, Barry Song wrote: > On Fri, Jul 26, 2024 at 10:45 PM Ryan Roberts <ryan.roberts@arm.com> wrote: >> >> On 26/07/2024 11:04, Barry Song wrote: >>> On Fri, Jul 26, 2024 at 9:48 PM David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 26.07.24 11:43, Ryan Roberts wrote: >>>>> On 26/07/2024 09:28, Barry Song wrote: >>>>>> From: Barry Song <v-songbaohua@oppo.com> >>>>>> >>>>>> khugepaged will be automatically started when PMD-sized THP is enabled >>>>>> (either of the per-size anon control or the top-level control are set >>>>>> to "always" or "madvise"), and it'll be automatically shutdown when >>>>>> PMD-sized THP is disabled (when both the per-size anon control and the >>>>>> top-level control are "never"). >>>>>> >>>>>> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP, >>>>>> as it would only waste CPU time. >>>>>> >>>>>> Cc: Lance Yang <ioworker0@gmail.com> >>>>>> Cc: Ryan Roberts <ryan.roberts@arm.com> >>>>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> >>>>>> Cc: David Hildenbrand <david@redhat.com> >>>>>> Cc: Yang Shi <shy828301@gmail.com> >>>>>> Cc: Zi Yan <ziy@nvidia.com> >>>>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com> >>>>>> --- >>>>>> mm/huge_memory.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>>>> index 41460847988c..bd365e35acf7 100644 >>>>>> --- a/mm/huge_memory.c >>>>>> +++ b/mm/huge_memory.c >>>>>> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, >>>>>> } else >>>>>> ret = -EINVAL; >>>>>> >>>>>> - if (ret > 0) { >>>>>> + if (ret > 0 && order == HPAGE_PMD_ORDER) { >>>>>> int err; >>>>>> >>>>>> err = start_stop_khugepaged(); >>>>> >>>>> Personally I see this as a bit of a layering violation; its >>>>> start_stop_khugepaged() that should decide the policy for when to start and stop >>>>> the daemon. thpsize_enabled_store() should just be calling >>>>> start_stop_khugepaged() to notify that something potentially pertinent to the a >>>>> policy decision has changed. >>>> >>> >>> My impression is that it slightly deviates from the huge page documentation in >>> Documentation/admin-guide/mm/transhuge.rst. >>> >>> khugepaged will be automatically started when PMD-sized THP is enabled >>> (either of the per-size anon control or the top-level control are set >>> to "always" or "madvise"), and it'll be automatically shutdown when >>> PMD-sized THP is disabled (when both the per-size anon control and the >>> top-level control are "never"). >> >> But start_stop_khugepaged() doesn't unconditionally start khugepaged, it takes >> action based on hugepage_pmd_enabled() which only returns true if there are any >> pmd sized THP enabled (currently looking at anon and file, but should also look >> at shmem in future; that's a known bug that's been there forever). So I don't >> think it is inconsistent with the documentation? > > My point is that, as a code reader, if non-PMD sizes are never > involved with khugepaged, > we can proceed without needing to check the lower-layer code. > otherwise it is a bit > confusing, especially since start_stop_khugepaged() unconditionally calls > set_recommended_min_free_kbytes() for all sizes, regardless of whether > they are PMD. > > However, I agree with your point that if non-PMD sizes are eventually managed by > khugepaged, then this approach is flawed. This is the downside of this change. Yes, I'd hope that eventually khugepaged will support non-PMD sizes. That's something we are hoping to look at later this year. So my personal preference is to leave as is. > >> >>> >>> non-PMD size is not involved in khugepaged, but I agree the policy might change >>> in the future. >>> >>>> Agreed, skimming the subject I was under the impression that we would be >>>> fixing something here. >>> >>> working on another swapin_enabled and reviewing the enabled source code. >>> I don't need this startstop for all sizes in that case, so I made a >>> quick adjustment >>> to this part as well. If neither of you likes it, that's fine with me :-) >>> >>>> >>>> -- >>>> Cheers, >>>> >>>> David / dhildenb >>>> > > Thanks > Barry
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 41460847988c..bd365e35acf7 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj, } else ret = -EINVAL; - if (ret > 0) { + if (ret > 0 && order == HPAGE_PMD_ORDER) { int err; err = start_stop_khugepaged();