[PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
the IRQ context. but drain_pages_zone fails to clear away the irq. because
preempt_disable have take effect. so it safely remove the code.

Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 mm/page_alloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 05c3956..7b16095 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2294,11 +2294,9 @@ void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp)
  */
 static void drain_pages_zone(unsigned int cpu, struct zone *zone)
 {
-	unsigned long flags;
 	struct per_cpu_pageset *pset;
 	struct per_cpu_pages *pcp;
 
-	local_irq_save(flags);
 	pset = per_cpu_ptr(zone->pageset, cpu);
 
 	pcp = &pset->pcp;
@@ -2306,7 +2304,6 @@ static void drain_pages_zone(unsigned int cpu, struct zone *zone)
 		free_pcppages_bulk(zone, pcp->count, pcp);
 		pcp->count = 0;
 	}
-	local_irq_restore(flags);
 }
 
 /*
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[Vlastimil Babka]

On 03/13/2017 09:02 AM, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
> the IRQ context. but drain_pages_zone fails to clear away the irq. because
> preempt_disable have take effect. so it safely remove the code.
> 
> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  mm/page_alloc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 05c3956..7b16095 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2294,11 +2294,9 @@ void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp)
>   */
>  static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>  {
> -	unsigned long flags;
>  	struct per_cpu_pageset *pset;
>  	struct per_cpu_pages *pcp;
>  
> -	local_irq_save(flags);
>  	pset = per_cpu_ptr(zone->pageset, cpu);

NAK. we have to make sure that pset corresponds to the cpu we are
running on.

>  
>  	pcp = &pset->pcp;
> @@ -2306,7 +2304,6 @@ static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>  		free_pcppages_bulk(zone, pcp->count, pcp);
>  		pcp->count = 0;
>  	}
> -	local_irq_restore(flags);
>  }
>  
>  /*
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[Vlastimil Babka]

On 03/13/2017 09:31 AM, Vlastimil Babka wrote:
> On 03/13/2017 09:02 AM, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
>> the IRQ context. but drain_pages_zone fails to clear away the irq. because
>> preempt_disable have take effect. so it safely remove the code.
>>
>> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/page_alloc.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 05c3956..7b16095 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2294,11 +2294,9 @@ void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp)
>>   */
>>  static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>>  {
>> -	unsigned long flags;
>>  	struct per_cpu_pageset *pset;
>>  	struct per_cpu_pages *pcp;
>>  
>> -	local_irq_save(flags);
>>  	pset = per_cpu_ptr(zone->pageset, cpu);
> 
> NAK. we have to make sure that pset corresponds to the cpu we are
> running on.

Sorry, I was thinking about other callers, such as drain_local_pages(),
but seems like all have the cpu pinned prerequisity.

But do we know that there can't be an interrupt updating pcp->count
between the moment we read it and we call free_pcppages_bulk? This
should be also discussed in the changelog. Also the "Fixes:" tag is not
necessary for a performance optimization.

> 
>>  
>>  	pcp = &pset->pcp;
>> @@ -2306,7 +2304,6 @@ static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>>  		free_pcppages_bulk(zone, pcp->count, pcp);
>>  		pcp->count = 0;
>>  	}
>> -	local_irq_restore(flags);
>>  }
>>  
>>  /*
>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[zhong jiang]

On 2017/3/13 18:08, Vlastimil Babka wrote:
> On 03/13/2017 09:31 AM, Vlastimil Babka wrote:
>> On 03/13/2017 09:02 AM, zhongjiang wrote:
>>> From: zhong jiang <zhongjiang@huawei.com>
>>>
>>> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
>>> the IRQ context. but drain_pages_zone fails to clear away the irq. because
>>> preempt_disable have take effect. so it safely remove the code.
>>>
>>> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>> ---
>>>  mm/page_alloc.c | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 05c3956..7b16095 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -2294,11 +2294,9 @@ void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp)
>>>   */
>>>  static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>>>  {
>>> -	unsigned long flags;
>>>  	struct per_cpu_pageset *pset;
>>>  	struct per_cpu_pages *pcp;
>>>  
>>> -	local_irq_save(flags);
>>>  	pset = per_cpu_ptr(zone->pageset, cpu);
>> NAK. we have to make sure that pset corresponds to the cpu we are
>> running on.
> Sorry, I was thinking about other callers, such as drain_local_pages(),
> but seems like all have the cpu pinned prerequisity.
>
> But do we know that there can't be an interrupt updating pcp->count
> between the moment we read it and we call free_pcppages_bulk? This
> should be also discussed in the changelog. Also the "Fixes:" tag is not
> necessary for a performance optimization.
 The title is misunderstanding. it is just for performence optimization.
 I will resent it in v2.

 Thanks
 zhongjiang
>>>  
>>>  	pcp = &pset->pcp;
>>> @@ -2306,7 +2304,6 @@ static void drain_pages_zone(unsigned int cpu, struct zone *zone)
>>>  		free_pcppages_bulk(zone, pcp->count, pcp);
>>>  		pcp->count = 0;
>>>  	}
>>> -	local_irq_restore(flags);
>>>  }
>>>  
>>>  /*
>>>
>
> .
>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[Mel Gorman]

On Mon, Mar 13, 2017 at 04:02:54PM +0800, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
> the IRQ context. but drain_pages_zone fails to clear away the irq. because
> preempt_disable have take effect. so it safely remove the code.
> 
> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>

It's not really a fix but is this even measurable?

The reason the IRQ saving was preserved was for callers that are removing
the CPU where it's not 100% clear if the CPU is protected from IPIs at
the time the pcpu drain takes place. It may be ok but the changelog
should include an indication that it has been considered and is known to
be fine versus CPU hotplug.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[zhong jiang]

On 2017/3/13 19:19, Mel Gorman wrote:
> On Mon, Mar 13, 2017 at 04:02:54PM +0800, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
>> the IRQ context. but drain_pages_zone fails to clear away the irq. because
>> preempt_disable have take effect. so it safely remove the code.
>>
>> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> It's not really a fix but is this even measurable?
>
> The reason the IRQ saving was preserved was for callers that are removing
> the CPU where it's not 100% clear if the CPU is protected from IPIs at
> the time the pcpu drain takes place. It may be ok but the changelog
> should include an indication that it has been considered and is known to
> be fine versus CPU hotplug.
>
you mean the removing cpu maybe  handle the IRQ, it will result in the incorrect pcpu->count ?

but I don't sure that dying cpu remain handle the IRQ.

Thanks
zhongjinag

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[Mel Gorman]

On Mon, Mar 13, 2017 at 09:59:33PM +0800, zhong jiang wrote:
> On 2017/3/13 19:19, Mel Gorman wrote:
> > On Mon, Mar 13, 2017 at 04:02:54PM +0800, zhongjiang wrote:
> >> From: zhong jiang <zhongjiang@huawei.com>
> >>
> >> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> >> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
> >> the IRQ context. but drain_pages_zone fails to clear away the irq. because
> >> preempt_disable have take effect. so it safely remove the code.
> >>
> >> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> > It's not really a fix but is this even measurable?
> >
> > The reason the IRQ saving was preserved was for callers that are removing
> > the CPU where it's not 100% clear if the CPU is protected from IPIs at
> > the time the pcpu drain takes place. It may be ok but the changelog
> > should include an indication that it has been considered and is known to
> > be fine versus CPU hotplug.
> >
> you mean the removing cpu maybe  handle the IRQ, it will result in the incorrect pcpu->count ?
> 

Yes, if it hasn't had interrupts disabled yet at the time of the drain.
I didn't check, it probably is called from a context that disables
interrupts but the fact you're not sure makes me automatically wary of
the patch particularly given how little difference it makes for the common
case where direct reclaim failed triggering a drain.

> but I don't sure that dying cpu remain handle the IRQ.
> 

You'd need to be certain to justify the patch.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[zhong jiang]

On 2017/3/13 22:26, Mel Gorman wrote:
> On Mon, Mar 13, 2017 at 09:59:33PM +0800, zhong jiang wrote:
>> On 2017/3/13 19:19, Mel Gorman wrote:
>>> On Mon, Mar 13, 2017 at 04:02:54PM +0800, zhongjiang wrote:
>>>> From: zhong jiang <zhongjiang@huawei.com>
>>>>
>>>> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>>> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
>>>> the IRQ context. but drain_pages_zone fails to clear away the irq. because
>>>> preempt_disable have take effect. so it safely remove the code.
>>>>
>>>> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>> It's not really a fix but is this even measurable?
>>>
>>> The reason the IRQ saving was preserved was for callers that are removing
>>> the CPU where it's not 100% clear if the CPU is protected from IPIs at
>>> the time the pcpu drain takes place. It may be ok but the changelog
>>> should include an indication that it has been considered and is known to
>>> be fine versus CPU hotplug.
>>>
>> you mean the removing cpu maybe  handle the IRQ, it will result in the incorrect pcpu->count ?
>>
> Yes, if it hasn't had interrupts disabled yet at the time of the drain.
> I didn't check, it probably is called from a context that disables
> interrupts but the fact you're not sure makes me automatically wary of
> the patch particularly given how little difference it makes for the common
> case where direct reclaim failed triggering a drain.
  Ok, I will test the benefits or not when direct reclaim failed and trigger a drain.  
>> but I don't sure that dying cpu remain handle the IRQ.
>>
> You'd need to be certain to justify the patch.
>
 Truely , Still  undecided it i rational at least theretically.  

Thanks
zhongjiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[zhong jiang]

On 2017/3/13 22:26, Mel Gorman wrote:
> On Mon, Mar 13, 2017 at 09:59:33PM +0800, zhong jiang wrote:
>> On 2017/3/13 19:19, Mel Gorman wrote:
>>> On Mon, Mar 13, 2017 at 04:02:54PM +0800, zhongjiang wrote:
>>>> From: zhong jiang <zhongjiang@huawei.com>
>>>>
>>>> when commit 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>>> introduced to the mainline, free_pcppages_bulk irq_save/resave to protect
>>>> the IRQ context. but drain_pages_zone fails to clear away the irq. because
>>>> preempt_disable have take effect. so it safely remove the code.
>>>>
>>>> Fixes: 374ad05ab64d ("mm, page_alloc: only use per-cpu allocator for irq-safe requests")
>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>> It's not really a fix but is this even measurable?
>>>
>>> The reason the IRQ saving was preserved was for callers that are removing
>>> the CPU where it's not 100% clear if the CPU is protected from IPIs at
>>> the time the pcpu drain takes place. It may be ok but the changelog
>>> should include an indication that it has been considered and is known to
>>> be fine versus CPU hotplug.
>>>
>> you mean the removing cpu maybe  handle the IRQ, it will result in the incorrect pcpu->count ?
>>
> Yes, if it hasn't had interrupts disabled yet at the time of the drain.
> I didn't check, it probably is called from a context that disables
> interrupts but the fact you're not sure makes me automatically wary of
> the patch particularly given how little difference it makes for the common
> case where direct reclaim failed triggering a drain.
>
>> but I don't sure that dying cpu remain handle the IRQ.
>>
> You'd need to be certain to justify the patch.
>
 Hi,  Mel
   
    by code review,  I see the cpu hotplug will only register the notfier to callback the function without
  interrupt come.  is it right ??

Thanks
zhongjiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm, page_alloc: fix the duplicate save/ressave irq

[Mel Gorman]

On Tue, Mar 14, 2017 at 08:06:34PM +0800, zhong jiang wrote:
> >> you mean the removing cpu maybe  handle the IRQ, it will result in the incorrect pcpu->count ?
> >>
> > Yes, if it hasn't had interrupts disabled yet at the time of the drain.
> > I didn't check, it probably is called from a context that disables
> > interrupts but the fact you're not sure makes me automatically wary of
> > the patch particularly given how little difference it makes for the common
> > case where direct reclaim failed triggering a drain.
> >
> >> but I don't sure that dying cpu remain handle the IRQ.
> >>
> > You'd need to be certain to justify the patch.
> >
>  Hi,  Mel
>    
>     by code review,  I see the cpu hotplug will only register the notfier to callback the function without
>   interrupt come.  is it right ??
> 

That sentence is impossible to parse meaningfully. The patch was
posted without verifying it made any performance difference (it almost
certainly won't) and without checking all the callers are actually safe
(which you're still not sure of).

Consider the patch NAK'd on both grounds (marginal, if any improvement
with no checking that the patch is safe).

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>