If "page" is the list head, list_for_each_entry_safe() would stop
iteration.
Signed-off-by: Wei Yang <[email protected]>
---
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 */
--
2.20.1 (Apple Git-117)
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 <[email protected]>
> ---
> 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?
--
Thanks,
David / dhildenb
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 <[email protected]>
>>> ---
>>> 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.
--
Thanks,
David / dhildenb
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 <[email protected]>
>> ---
>> 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 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 <[email protected]>
>> ---
>> 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
--
Wei Yang
Help you, Help me
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 <[email protected]>
>>>> ---
>>>> 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
--
Wei Yang
Help you, Help me
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 <[email protected]>
>> > ---
>> > 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 :-)
--
Wei Yang
Help you, Help me
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 <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
>>> 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 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 <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>
>
>>>> 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
--
Wei Yang
Help you, Help me
On Mon, Aug 17, 2020 at 8:22 PM Wei Yang
<[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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