Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570109 > unrolled thread

Re: [RFC V2 05/12] cpuset: Add cpuset_inc() inside cpuset_init()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC V2 05/12] cpuset: Add cpuset_inc() inside cpuset_init() Mel Gorman <mgorman@suse.de> - 2017-01-30 21:50 +0100
    [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-01-31 15:30 +0100
      Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed  nodemask Mel Gorman <mgorman@suse.de> - 2017-01-31 17:10 +0100
        Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed  nodemask Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-02-01 08:40 +0100
          Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed  nodemask Michal Hocko <mhocko@kernel.org> - 2017-02-01 10:00 +0100
          Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed  nodemask Mel Gorman <mgorman@suse.de> - 2017-02-01 10:20 +0100
    Re: [RFC V2 05/12] cpuset: Add cpuset_inc() inside cpuset_init() Vlastimil Babka <vbabka@suse.cz> - 2017-01-31 15:40 +0100
      Re: [RFC V2 05/12] cpuset: Add cpuset_inc() inside cpuset_init() Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-01-31 16:40 +0100

#1570109 — Re: [RFC V2 05/12] cpuset: Add cpuset_inc() inside cpuset_init()

On Mon, Jan 30, 2017 at 09:05:46AM +0530, Anshuman Khandual wrote:
> Currently cpusets_enabled() wrongfully returns 0 even if we have a root
> cpuset configured on the system. This got missed when jump level was
> introduced in place of number_of_cpusets with the commit 664eeddeef65
> ("mm: page_alloc: use jump labels to avoid checking number_of_cpusets")
> . This fixes the problem so that cpusets_enabled() returns positive even
> for the root cpuset.
> 
> Fixes: 664eeddeef65 ("mm: page_alloc: use jump labels to avoid")
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Superficially, this appears to always activate the cpuset_enabled
branch() when it doesn't really make sense that the root cpuset be
restricted. I strongly suspect it should be altered to cpuset_inc only
if the root cpuset is configured to isolate memory.

-- 
Mel Gorman
SUSE Labs

[toc] | [next] | [standalone]

#1570784 — [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask

At present, top_cpuset.mems_allowed is same as node_states[N_MEMORY] and it
cannot be changed at the runtime. Maximum possible node_states[N_MEMORY]
also gets reflected in top_cpuset.effective_mems interface. It prevents some
one from removing or restricting memory placement which will be applicable
system wide on a given memory node through cpuset mechanism which might be
limiting. This solves the problem by enabling update_nodemask() function to
accept changes to top_cpuset.mems_allowed as well. Once changed, it also
updates the value of top_cpuset.effective_mems. Updates all it's task's
mems_allowed nodemask as well. It calls cpuset_inc() to make sure cpuset
is accounted for in the buddy allocator through cpusets_enabled() check.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Tested for

* Enforcement of changed top_cpuset.mems_allowed
* Global mems_allowed cannot be changed till there are other
  cpusets present underneath the top root cpuset. I guess it
  is expected.

 kernel/cpuset.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index b308888..e8c105a 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1210,15 +1210,6 @@ static int update_nodemask(struct cpuset *cs, struct cpuset *trialcs,
 	int retval;
 
 	/*
-	 * top_cpuset.mems_allowed tracks node_stats[N_MEMORY];
-	 * it's read-only
-	 */
-	if (cs == &top_cpuset) {
-		retval = -EACCES;
-		goto done;
-	}
-
-	/*
 	 * An empty mems_allowed is ok iff there are no tasks in the cpuset.
 	 * Since nodelist_parse() fails on an empty mask, we special case
 	 * that parsing.  The validate_change() call ensures that cpusets
@@ -1232,7 +1223,7 @@ static int update_nodemask(struct cpuset *cs, struct cpuset *trialcs,
 			goto done;
 
 		if (!nodes_subset(trialcs->mems_allowed,
-				  top_cpuset.mems_allowed)) {
+				  node_states[N_MEMORY])) {
 			retval = -EINVAL;
 			goto done;
 		}
@@ -1250,6 +1241,16 @@ static int update_nodemask(struct cpuset *cs, struct cpuset *trialcs,
 	cs->mems_allowed = trialcs->mems_allowed;
 	spin_unlock_irq(&callback_lock);
 
+	if (cs == &top_cpuset) {
+		spin_lock_irq(&callback_lock);
+		cs->effective_mems = trialcs->mems_allowed;
+		spin_unlock_irq(&callback_lock);
+
+		update_tasks_nodemask(cs);
+		cpuset_inc();
+		goto done;
+	}
+
 	/* use trialcs->mems_allowed as a temp variable */
 	update_nodemasks_hier(cs, &trialcs->mems_allowed);
 done:
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1570857 — Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask

On Tue, Jan 31, 2017 at 07:52:37PM +0530, Anshuman Khandual wrote:
> At present, top_cpuset.mems_allowed is same as node_states[N_MEMORY] and it
> cannot be changed at the runtime. Maximum possible node_states[N_MEMORY]
> also gets reflected in top_cpuset.effective_mems interface. It prevents some
> one from removing or restricting memory placement which will be applicable
> system wide on a given memory node through cpuset mechanism which might be
> limiting. This solves the problem by enabling update_nodemask() function to
> accept changes to top_cpuset.mems_allowed as well. Once changed, it also
> updates the value of top_cpuset.effective_mems. Updates all it's task's
> mems_allowed nodemask as well. It calls cpuset_inc() to make sure cpuset
> is accounted for in the buddy allocator through cpusets_enabled() check.
> 

What's the point of allowing the root cpuset to be restricted?

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1571296 — Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask

On 01/31/2017 09:30 PM, Mel Gorman wrote:
> On Tue, Jan 31, 2017 at 07:52:37PM +0530, Anshuman Khandual wrote:
>> At present, top_cpuset.mems_allowed is same as node_states[N_MEMORY] and it
>> cannot be changed at the runtime. Maximum possible node_states[N_MEMORY]
>> also gets reflected in top_cpuset.effective_mems interface. It prevents some
>> one from removing or restricting memory placement which will be applicable
>> system wide on a given memory node through cpuset mechanism which might be
>> limiting. This solves the problem by enabling update_nodemask() function to
>> accept changes to top_cpuset.mems_allowed as well. Once changed, it also
>> updates the value of top_cpuset.effective_mems. Updates all it's task's
>> mems_allowed nodemask as well. It calls cpuset_inc() to make sure cpuset
>> is accounted for in the buddy allocator through cpusets_enabled() check.
>>
> 
> What's the point of allowing the root cpuset to be restricted?

After an extended period of run time on a system, currently if we have
to run HW diagnostics and dump (which are run out of band) for debug
purpose, we have to stop further allocations to the node. Hot plugging
the memory node out of the kernel will achieve this. But it can also
be made possible by just enabling top_cpuset.memory_migrate and then
restricting all the allocations by removing the node from top_cpuset.
mems_allowed nodemask. This will force all the existing allocations
out of the target node.

More importantly it also extends the cpuset memory restriction feature
to the logical completion without adding any regressions for the
existing use cases. Then why not do this ? Does it add any overhead ?

In the future this feature can also be used to isolate a memory node
from all possible general allocations and at the same time provide an
alternate method for explicit allocation into it (still working on this
part, though have a hack right now). The current RFC series proposes
one such possible use case through the top_cpuset.mems_allowed nodemask.
But in this case it is being restricted during boot as well as after
hotplug of a memory only NUMA node.

If you think currently this does not have a use case to stand on it's
own, then I will carry it along with this patch series as part of the
proposed cpuset based isolation solution (with explicit allocation
access to the isolated node) as described just above.

- Anshuman

[toc] | [prev] | [next] | [standalone]

#1571337 — Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask

On Wed 01-02-17 13:01:24, Anshuman Khandual wrote:
[...]
> More importantly it also extends the cpuset memory restriction feature
> to the logical completion without adding any regressions for the
> existing use cases. Then why not do this ? Does it add any overhead ?

Maybe it doesn't add any overhead but it just breaks the cgroups
expectation that the root cgroup covers the full resource set. No cgroup
controller allows to set limits on the root cgroup. So all this looks
like an abuse of the interface.

I haven't read the full series yet but this particular change looks like
a nogo to me.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1571348 — Re: [RFC] cpuset: Enable changing of top_cpuset's mems_allowed nodemask

On Wed, Feb 01, 2017 at 01:01:24PM +0530, Anshuman Khandual wrote:
> On 01/31/2017 09:30 PM, Mel Gorman wrote:
> > On Tue, Jan 31, 2017 at 07:52:37PM +0530, Anshuman Khandual wrote:
> >> At present, top_cpuset.mems_allowed is same as node_states[N_MEMORY] and it
> >> cannot be changed at the runtime. Maximum possible node_states[N_MEMORY]
> >> also gets reflected in top_cpuset.effective_mems interface. It prevents some
> >> one from removing or restricting memory placement which will be applicable
> >> system wide on a given memory node through cpuset mechanism which might be
> >> limiting. This solves the problem by enabling update_nodemask() function to
> >> accept changes to top_cpuset.mems_allowed as well. Once changed, it also
> >> updates the value of top_cpuset.effective_mems. Updates all it's task's
> >> mems_allowed nodemask as well. It calls cpuset_inc() to make sure cpuset
> >> is accounted for in the buddy allocator through cpusets_enabled() check.
> >>
> > 
> > What's the point of allowing the root cpuset to be restricted?
> 
> After an extended period of run time on a system, currently if we have
> to run HW diagnostics and dump (which are run out of band) for debug
> purpose, we have to stop further allocations to the node. Hot plugging
> the memory node out of the kernel will achieve this. But it can also
> be made possible by just enabling top_cpuset.memory_migrate and then
> restricting all the allocations by removing the node from top_cpuset.
> mems_allowed nodemask. This will force all the existing allocations
> out of the target node.
> 

So would creating a restricted cpuset and migrating all tasks from the
root cpuset into it.

> More importantly it also extends the cpuset memory restriction feature
> to the logical completion without adding any regressions for the
> existing use cases. Then why not do this ? Does it add any overhead ?
> 

It violates the expectation that the root cgroup can access all
resources. Once enabled, there is some overhead in the page allocator as
it must check all cpusets even for tasks that weren't configured to be
isolated.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1570790

On 01/30/2017 09:30 PM, Mel Gorman wrote:
> On Mon, Jan 30, 2017 at 09:05:46AM +0530, Anshuman Khandual wrote:
>> Currently cpusets_enabled() wrongfully returns 0 even if we have a root
>> cpuset configured on the system. This got missed when jump level was
>> introduced in place of number_of_cpusets with the commit 664eeddeef65
>> ("mm: page_alloc: use jump labels to avoid checking number_of_cpusets")
>> . This fixes the problem so that cpusets_enabled() returns positive even
>> for the root cpuset.
>>
>> Fixes: 664eeddeef65 ("mm: page_alloc: use jump labels to avoid")
>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> 
> Superficially, this appears to always activate the cpuset_enabled
> branch() when it doesn't really make sense that the root cpuset be
> restricted.

Yes that's why root cpuset doesn't "count", as it's not supposed to be
restricted (it's also documented in cpusets.txt) Thus the "Fixes:" tag
is very misleading.

> I strongly suspect it should be altered to cpuset_inc only
> if the root cpuset is configured to isolate memory.
> 

[toc] | [prev] | [next] | [standalone]

#1570832

On 01/31/2017 08:06 PM, Vlastimil Babka wrote:
> On 01/30/2017 09:30 PM, Mel Gorman wrote:
>> On Mon, Jan 30, 2017 at 09:05:46AM +0530, Anshuman Khandual wrote:
>>> Currently cpusets_enabled() wrongfully returns 0 even if we have a root
>>> cpuset configured on the system. This got missed when jump level was
>>> introduced in place of number_of_cpusets with the commit 664eeddeef65
>>> ("mm: page_alloc: use jump labels to avoid checking number_of_cpusets")
>>> . This fixes the problem so that cpusets_enabled() returns positive even
>>> for the root cpuset.
>>>
>>> Fixes: 664eeddeef65 ("mm: page_alloc: use jump labels to avoid")
>>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>
>> Superficially, this appears to always activate the cpuset_enabled
>> branch() when it doesn't really make sense that the root cpuset be
>> restricted.
> 
> Yes that's why root cpuset doesn't "count", as it's not supposed to be
> restricted (it's also documented in cpusets.txt) Thus the "Fixes:" tag
> is very misleading.

Agreed, I have removed the "Fixes: " tag in the proposed RFC already
posted on this thread where it puts it as a new enablement instead
and an addition to the capability what we already have with cpuset.
It will be great if you can please take a look and provide feedback.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web