| Message ID | 20200817084836.29216-1-richard.weiyang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_reporting: the "page" must not be the list head | expand |
On 17.08.20 10:48, Wei Yang wrote: > If "page" is the list head, list_for_each_entry_safe() would stop > iteration. > > Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > --- > mm/page_reporting.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page_reporting.c b/mm/page_reporting.c > index 3bbd471cfc81..aaaa3605123d 100644 > --- a/mm/page_reporting.c > +++ b/mm/page_reporting.c > @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, > * the new head of the free list before we release the > * zone lock. > */ > - if (&page->lru != list && !list_is_first(&page->lru, list)) > + if (!list_is_first(&page->lru, list)) > list_rotate_to_front(&page->lru, list); > > /* release lock before waiting on report processing */ > Is this a fix or a cleanup? If it's a fix, can this be reproduced easily and what ere the effects?
On 8/17/2020 2:35 AM, David Hildenbrand wrote: > On 17.08.20 10:48, Wei Yang wrote: >> If "page" is the list head, list_for_each_entry_safe() would stop >> iteration. >> >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> --- >> mm/page_reporting.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >> index 3bbd471cfc81..aaaa3605123d 100644 >> --- a/mm/page_reporting.c >> +++ b/mm/page_reporting.c >> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >> * the new head of the free list before we release the >> * zone lock. >> */ >> - if (&page->lru != list && !list_is_first(&page->lru, list)) >> + if (!list_is_first(&page->lru, list)) >> list_rotate_to_front(&page->lru, list); >> >> /* release lock before waiting on report processing */ >> > > Is this a fix or a cleanup? If it's a fix, can this be reproduced easily > and what ere the effects? > This should be a clean-up. Since the &page->lru != list will always be true. If I recall at some point the that was a check for &next->lru != list but I think I pulled out an additional conditional check somewhere so that we just go through the start of the loop again and iterate over reported pages until we are guaranteed to have a non-reported page to rotate to the top of the list with the general idea being that we wanted the allocator to pull non-reported pages before reported pages.
On 17.08.20 18:05, Alexander Duyck wrote: > > > On 8/17/2020 2:35 AM, David Hildenbrand wrote: >> On 17.08.20 10:48, Wei Yang wrote: >>> If "page" is the list head, list_for_each_entry_safe() would stop >>> iteration. >>> >>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>> --- >>> mm/page_reporting.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >>> index 3bbd471cfc81..aaaa3605123d 100644 >>> --- a/mm/page_reporting.c >>> +++ b/mm/page_reporting.c >>> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >>> * the new head of the free list before we release the >>> * zone lock. >>> */ >>> - if (&page->lru != list && !list_is_first(&page->lru, list)) >>> + if (!list_is_first(&page->lru, list)) >>> list_rotate_to_front(&page->lru, list); >>> >>> /* release lock before waiting on report processing */ >>> >> >> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >> and what ere the effects? >> > > This should be a clean-up. Since the &page->lru != list will always be true. > Makes sense, maybe we can make that a little bit clearer in the patch description. > If I recall at some point the that was a check for &next->lru != list > but I think I pulled out an additional conditional check somewhere so > that we just go through the start of the loop again and iterate over > reported pages until we are guaranteed to have a non-reported page to > rotate to the top of the list with the general idea being that we wanted > the allocator to pull non-reported pages before reported pages.
On Mon, Aug 17, 2020 at 11:35:29AM +0200, David Hildenbrand wrote: >On 17.08.20 10:48, Wei Yang wrote: >> If "page" is the list head, list_for_each_entry_safe() would stop >> iteration. >> >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> --- >> mm/page_reporting.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >> index 3bbd471cfc81..aaaa3605123d 100644 >> --- a/mm/page_reporting.c >> +++ b/mm/page_reporting.c >> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >> * the new head of the free list before we release the >> * zone lock. >> */ >> - if (&page->lru != list && !list_is_first(&page->lru, list)) >> + if (!list_is_first(&page->lru, list)) >> list_rotate_to_front(&page->lru, list); >> >> /* release lock before waiting on report processing */ >> > >Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >and what ere the effects? > I think this is a cleanup. I am not sure why you ask this, since the check must be true when the iteration continues. >-- >Thanks, > >David / dhildenb
On Mon, Aug 17, 2020 at 07:07:04PM +0200, David Hildenbrand wrote: >On 17.08.20 18:05, Alexander Duyck wrote: >> >> >> On 8/17/2020 2:35 AM, David Hildenbrand wrote: >>> On 17.08.20 10:48, Wei Yang wrote: >>>> If "page" is the list head, list_for_each_entry_safe() would stop >>>> iteration. >>>> >>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>>> --- >>>> mm/page_reporting.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >>>> index 3bbd471cfc81..aaaa3605123d 100644 >>>> --- a/mm/page_reporting.c >>>> +++ b/mm/page_reporting.c >>>> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >>>> * the new head of the free list before we release the >>>> * zone lock. >>>> */ >>>> - if (&page->lru != list && !list_is_first(&page->lru, list)) >>>> + if (!list_is_first(&page->lru, list)) >>>> list_rotate_to_front(&page->lru, list); >>>> >>>> /* release lock before waiting on report processing */ >>>> >>> >>> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >>> and what ere the effects? >>> >> >> This should be a clean-up. Since the &page->lru != list will always be true. >> > >Makes sense, maybe we can make that a little bit clearer in the patch >description. > Ok, do you have some suggestion on the description? A clean-up for commit xxx? I would appreciate your suggestion :-) >> If I recall at some point the that was a check for &next->lru != list >> but I think I pulled out an additional conditional check somewhere so >> that we just go through the start of the loop again and iterate over >> reported pages until we are guaranteed to have a non-reported page to >> rotate to the top of the list with the general idea being that we wanted >> the allocator to pull non-reported pages before reported pages. > >-- >Thanks, > >David / dhildenb
On Mon, Aug 17, 2020 at 09:05:32AM -0700, Alexander Duyck wrote: > > >On 8/17/2020 2:35 AM, David Hildenbrand wrote: >> On 17.08.20 10:48, Wei Yang wrote: >> > If "page" is the list head, list_for_each_entry_safe() would stop >> > iteration. >> > >> > Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> > --- >> > mm/page_reporting.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/mm/page_reporting.c b/mm/page_reporting.c >> > index 3bbd471cfc81..aaaa3605123d 100644 >> > --- a/mm/page_reporting.c >> > +++ b/mm/page_reporting.c >> > @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >> > * the new head of the free list before we release the >> > * zone lock. >> > */ >> > - if (&page->lru != list && !list_is_first(&page->lru, list)) >> > + if (!list_is_first(&page->lru, list)) >> > list_rotate_to_front(&page->lru, list); >> > /* release lock before waiting on report processing */ >> > >> >> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >> and what ere the effects? >> > >This should be a clean-up. Since the &page->lru != list will always be true. > >If I recall at some point the that was a check for &next->lru != list but I >think I pulled out an additional conditional check somewhere so that we just >go through the start of the loop again and iterate over reported pages until >we are guaranteed to have a non-reported page to rotate to the top of the >list with the general idea being that we wanted the allocator to pull >non-reported pages before reported pages. Hi, Alexander, I see you mentioned in the changelog, this change "mm/page_reporting: rotate reported pages to the tail of the list" brings some performance gain. Would you mind sharing more test detail? I would like to have a try at my side. Thanks :-)
On 18.08.20 05:05, Wei Yang wrote: > On Mon, Aug 17, 2020 at 07:07:04PM +0200, David Hildenbrand wrote: >> On 17.08.20 18:05, Alexander Duyck wrote: >>> >>> >>> On 8/17/2020 2:35 AM, David Hildenbrand wrote: >>>> On 17.08.20 10:48, Wei Yang wrote: >>>>> If "page" is the list head, list_for_each_entry_safe() would stop >>>>> iteration. >>>>> >>>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>>>> --- >>>>> mm/page_reporting.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >>>>> index 3bbd471cfc81..aaaa3605123d 100644 >>>>> --- a/mm/page_reporting.c >>>>> +++ b/mm/page_reporting.c >>>>> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >>>>> * the new head of the free list before we release the >>>>> * zone lock. >>>>> */ >>>>> - if (&page->lru != list && !list_is_first(&page->lru, list)) >>>>> + if (!list_is_first(&page->lru, list)) >>>>> list_rotate_to_front(&page->lru, list); >>>>> >>>>> /* release lock before waiting on report processing */ >>>>> >>>> >>>> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >>>> and what ere the effects? >>>> >>> >>> This should be a clean-up. Since the &page->lru != list will always be true. >>> >> >> Makes sense, maybe we can make that a little bit clearer in the patch >> description. >> > > Ok, do you have some suggestion on the description? > > A clean-up for commit xxx? > > I would appreciate your suggestion :-) > I'd go with something like " mm/page_reporting: drop stale list head check in page_reporting_cycle list_for_each_entry_safe() guarantees that we will never stumble over the list head; "&page->lru != list" will always evaluate to true. Let's simplify. " to stress that this is a pure simplifcation. Reviewed-by: David Hildenbrand <david@redhat.com> >>> If I recall at some point the that was a check for &next->lru != list >>> but I think I pulled out an additional conditional check somewhere so >>> that we just go through the start of the loop again and iterate over >>> reported pages until we are guaranteed to have a non-reported page to >>> rotate to the top of the list with the general idea being that we wanted >>> the allocator to pull non-reported pages before reported pages. >> >> -- >> Thanks, >> >> David / dhildenb >
On Tue, Aug 18, 2020 at 09:23:12AM +0200, David Hildenbrand wrote: >On 18.08.20 05:05, Wei Yang wrote: >> On Mon, Aug 17, 2020 at 07:07:04PM +0200, David Hildenbrand wrote: >>> On 17.08.20 18:05, Alexander Duyck wrote: >>>> >>>> >>>> On 8/17/2020 2:35 AM, David Hildenbrand wrote: >>>>> On 17.08.20 10:48, Wei Yang wrote: >>>>>> If "page" is the list head, list_for_each_entry_safe() would stop >>>>>> iteration. >>>>>> >>>>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>>>>> --- >>>>>> mm/page_reporting.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c >>>>>> index 3bbd471cfc81..aaaa3605123d 100644 >>>>>> --- a/mm/page_reporting.c >>>>>> +++ b/mm/page_reporting.c >>>>>> @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, >>>>>> * the new head of the free list before we release the >>>>>> * zone lock. >>>>>> */ >>>>>> - if (&page->lru != list && !list_is_first(&page->lru, list)) >>>>>> + if (!list_is_first(&page->lru, list)) >>>>>> list_rotate_to_front(&page->lru, list); >>>>>> >>>>>> /* release lock before waiting on report processing */ >>>>>> >>>>> >>>>> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily >>>>> and what ere the effects? >>>>> >>>> >>>> This should be a clean-up. Since the &page->lru != list will always be true. >>>> >>> >>> Makes sense, maybe we can make that a little bit clearer in the patch >>> description. >>> >> >> Ok, do you have some suggestion on the description? >> >> A clean-up for commit xxx? >> >> I would appreciate your suggestion :-) >> > >I'd go with something like > >" >mm/page_reporting: drop stale list head check in page_reporting_cycle > >list_for_each_entry_safe() guarantees that we will never stumble over >the list head; "&page->lru != list" will always evaluate to true. Let's >simplify. >" > Looks really better than mine. Thanks a lot. >to stress that this is a pure simplifcation. > >Reviewed-by: David Hildenbrand <david@redhat.com> > >>>> If I recall at some point the that was a check for &next->lru != list >>>> but I think I pulled out an additional conditional check somewhere so >>>> that we just go through the start of the loop again and iterate over >>>> reported pages until we are guaranteed to have a non-reported page to >>>> rotate to the top of the list with the general idea being that we wanted >>>> the allocator to pull non-reported pages before reported pages. >>> >>> -- >>> Thanks, >>> >>> David / dhildenb >> > > >-- >Thanks, > >David / dhildenb
On Mon, Aug 17, 2020 at 8:22 PM Wei Yang <richard.weiyang@linux.alibaba.com> wrote: > > On Mon, Aug 17, 2020 at 09:05:32AM -0700, Alexander Duyck wrote: > > > > > >On 8/17/2020 2:35 AM, David Hildenbrand wrote: > >> On 17.08.20 10:48, Wei Yang wrote: > >> > If "page" is the list head, list_for_each_entry_safe() would stop > >> > iteration. > >> > > >> > Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > >> > --- > >> > mm/page_reporting.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/mm/page_reporting.c b/mm/page_reporting.c > >> > index 3bbd471cfc81..aaaa3605123d 100644 > >> > --- a/mm/page_reporting.c > >> > +++ b/mm/page_reporting.c > >> > @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, > >> > * the new head of the free list before we release the > >> > * zone lock. > >> > */ > >> > - if (&page->lru != list && !list_is_first(&page->lru, list)) > >> > + if (!list_is_first(&page->lru, list)) > >> > list_rotate_to_front(&page->lru, list); > >> > /* release lock before waiting on report processing */ > >> > > >> > >> Is this a fix or a cleanup? If it's a fix, can this be reproduced easily > >> and what ere the effects? > >> > > > >This should be a clean-up. Since the &page->lru != list will always be true. > > > >If I recall at some point the that was a check for &next->lru != list but I > >think I pulled out an additional conditional check somewhere so that we just > >go through the start of the loop again and iterate over reported pages until > >we are guaranteed to have a non-reported page to rotate to the top of the > >list with the general idea being that we wanted the allocator to pull > >non-reported pages before reported pages. > > Hi, Alexander, > > I see you mentioned in the changelog, this change "mm/page_reporting: rotate > reported pages to the tail of the list" brings some performance gain. > > Would you mind sharing more test detail? I would like to have a try at my > side. > > Thanks :-) I seem to recall my default test for most of this was the page_fault1 test from the will-it-scale suite of tests. Basically I was running that while leaving page reporting enabled. However I don't know how much visibility you would have into the performance impact as I seem to recall I had to modify the frequency of scheduling for the reporting polling task in order to see much of an impact. Thanks. - Alex
diff --git a/mm/page_reporting.c b/mm/page_reporting.c index 3bbd471cfc81..aaaa3605123d 100644 --- a/mm/page_reporting.c +++ b/mm/page_reporting.c @@ -178,7 +178,7 @@ page_reporting_cycle(struct page_reporting_dev_info *prdev, struct zone *zone, * the new head of the free list before we release the * zone lock. */ - if (&page->lru != list && !list_is_first(&page->lru, list)) + if (!list_is_first(&page->lru, list)) list_rotate_to_front(&page->lru, list); /* release lock before waiting on report processing */
If "page" is the list head, list_for_each_entry_safe() would stop iteration. Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> --- mm/page_reporting.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)