[PATCH] mm/page_reporting: the "page" must not be the list head

[PATCH] mm/page_reporting: the "page" must not be the list head

[Wei Yang]

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 */
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[David Hildenbrand]

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?

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Alexander Duyck]

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.

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[David Hildenbrand]

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.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Wei Yang]

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 :-)

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[David Hildenbrand]

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
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_reporting: the "page" must not be the list head

[Alexander Duyck]

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