From: Cody P Schafer <cody@linux.vnet.ibm.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Gilad Ben-Yossef <gilad@benyossef.com>,
Simon Jeons <simon.jeons@gmail.com>,
KOSAKI Motohiro <kosaki.motohiro@gmail.com>,
Mel Gorman <mgorman@suse.de>, Linux MM <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 00/11] mm: fixup changers of per cpu pageset's ->high and ->batch
Date: Wed, 10 Apr 2013 14:25:50 -0700 [thread overview]
Message-ID: <5165D8DE.5090801@linux.vnet.ibm.com> (raw)
In-Reply-To: <20130410142354.6044338fd68ff2ad165b1bc8@linux-foundation.org>
On 04/10/2013 02:23 PM, Andrew Morton wrote:
> On Wed, 10 Apr 2013 11:23:28 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
>> "Problems" with the current code:
>> 1. there is a lack of synchronization in setting ->high and ->batch in
>> percpu_pagelist_fraction_sysctl_handler()
>> 2. stop_machine() in zone_pcp_update() is unnecissary.
>> 3. zone_pcp_update() does not consider the case where percpu_pagelist_fraction is non-zero
>>
>> To fix:
>> 1. add memory barriers, a safe ->batch value, an update side mutex when
>> updating ->high and ->batch, and use ACCESS_ONCE() for ->batch users that
>> expect a stable value.
>> 2. avoid draining pages in zone_pcp_update(), rely upon the memory barriers added to fix #1
>> 3. factor out quite a few functions, and then call the appropriate one.
>>
>> Note that it results in a change to the behavior of zone_pcp_update(), which is
>> used by memory_hotplug. I'm rather certain that I've diserned (and preserved)
>> the essential behavior (changing ->high and ->batch), and only eliminated
>> unneeded actions (draining the per cpu pages), but this may not be the case.
>>
>> Further note that the draining of pages that previously took place in
>> zone_pcp_update() occured after repeated draining when attempting to offline a
>> page, and after the offline has "succeeded". It appears that the draining was
>> added to zone_pcp_update() to avoid refactoring setup_pageset() into 2
>> funtions.
>
> There hasn't been a ton of review activity for this patchset :(
>
> I'm inclined to duck it until after 3.9. Do the patches fix any
> noticeably bad userspace behavior?
No, all the bugs are theoretical. Waiting should be fine.
--
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>
WARNING: multiple messages have this Message-ID (diff)
From: Cody P Schafer <cody@linux.vnet.ibm.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Gilad Ben-Yossef <gilad@benyossef.com>,
Simon Jeons <simon.jeons@gmail.com>,
KOSAKI Motohiro <kosaki.motohiro@gmail.com>,
Mel Gorman <mgorman@suse.de>, Linux MM <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 00/11] mm: fixup changers of per cpu pageset's ->high and ->batch
Date: Wed, 10 Apr 2013 14:25:50 -0700 [thread overview]
Message-ID: <5165D8DE.5090801@linux.vnet.ibm.com> (raw)
In-Reply-To: <20130410142354.6044338fd68ff2ad165b1bc8@linux-foundation.org>
On 04/10/2013 02:23 PM, Andrew Morton wrote:
> On Wed, 10 Apr 2013 11:23:28 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
>> "Problems" with the current code:
>> 1. there is a lack of synchronization in setting ->high and ->batch in
>> percpu_pagelist_fraction_sysctl_handler()
>> 2. stop_machine() in zone_pcp_update() is unnecissary.
>> 3. zone_pcp_update() does not consider the case where percpu_pagelist_fraction is non-zero
>>
>> To fix:
>> 1. add memory barriers, a safe ->batch value, an update side mutex when
>> updating ->high and ->batch, and use ACCESS_ONCE() for ->batch users that
>> expect a stable value.
>> 2. avoid draining pages in zone_pcp_update(), rely upon the memory barriers added to fix #1
>> 3. factor out quite a few functions, and then call the appropriate one.
>>
>> Note that it results in a change to the behavior of zone_pcp_update(), which is
>> used by memory_hotplug. I'm rather certain that I've diserned (and preserved)
>> the essential behavior (changing ->high and ->batch), and only eliminated
>> unneeded actions (draining the per cpu pages), but this may not be the case.
>>
>> Further note that the draining of pages that previously took place in
>> zone_pcp_update() occured after repeated draining when attempting to offline a
>> page, and after the offline has "succeeded". It appears that the draining was
>> added to zone_pcp_update() to avoid refactoring setup_pageset() into 2
>> funtions.
>
> There hasn't been a ton of review activity for this patchset :(
>
> I'm inclined to duck it until after 3.9. Do the patches fix any
> noticeably bad userspace behavior?
No, all the bugs are theoretical. Waiting should be fine.
next prev parent reply other threads:[~2013-04-10 21:25 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-10 18:23 [PATCH v3 00/11] mm: fixup changers of per cpu pageset's ->high and ->batch Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 01/11] mm/page_alloc: factor out setting of pcp->high and pcp->batch Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 02/11] mm/page_alloc: prevent concurrent updaters of pcp ->batch and ->high Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 03/11] mm/page_alloc: insert memory barriers to allow async update of pcp batch and high Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 04/11] mm/page_alloc: protect pcp->batch accesses with ACCESS_ONCE Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 05/11] mm/page_alloc: convert zone_pcp_update() to rely on memory barriers instead of stop_machine() Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 06/11] mm/page_alloc: when handling percpu_pagelist_fraction, don't unneedly recalulate high Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 07/11] mm/page_alloc: factor setup_pageset() into pageset_init() and pageset_set_batch() Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 08/11] mm/page_alloc: relocate comment to be directly above code it refers to Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 09/11] mm/page_alloc: factor zone_pageset_init() out of setup_zone_pageset() Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 10/11] mm/page_alloc: in zone_pcp_update(), uze zone_pageset_init() Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 18:23 ` [PATCH v3 11/11] mm/page_alloc: rename setup_pagelist_highmark() to match naming of pageset_set_batch() Cody P Schafer
2013-04-10 18:23 ` Cody P Schafer
2013-04-10 21:23 ` [PATCH v3 00/11] mm: fixup changers of per cpu pageset's ->high and ->batch Andrew Morton
2013-04-10 21:23 ` Andrew Morton
2013-04-10 21:25 ` Cody P Schafer [this message]
2013-04-10 21:25 ` Cody P Schafer
2013-05-01 23:53 ` Cody P Schafer
2013-05-01 23:53 ` Cody P Schafer
2013-05-07 21:24 ` Andrew Morton
2013-05-07 21:24 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5165D8DE.5090801@linux.vnet.ibm.com \
--to=cody@linux.vnet.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=gilad@benyossef.com \
--cc=kosaki.motohiro@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=simon.jeons@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.