Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 25 Apr 2022 15:12:44 +0530
From:   Jagdish Gediya <jvgediya@linux.ibm.com>
To:     "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, baolin.wang@linux.alibaba.com,
        dave.hansen@linux.intel.com, ying.huang@intel.com,
        shy828301@gmail.com, weixugc@google.com, gthelen@google.com,
        dan.j.williams@intel.com
Subject: Re: [PATCH v3 3/7] drivers/base/node: Add support to write
 node_states[] via sysfs
Message-ID: <YmZtFDMJO5Kmt/0u@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
References: <20220422195516.10769-1-jvgediya@linux.ibm.com>
 <20220422195516.10769-4-jvgediya@linux.ibm.com>
 <87o80rgfpy.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87o80rgfpy.fsf@linux.ibm.com>
Precedence: bulk

On Sun, Apr 24, 2022 at 11:55:45AM +0530, Aneesh Kumar K.V wrote:
> Jagdish Gediya <jvgediya@linux.ibm.com> writes:
> 
> > Current /sys/devices/system/node/* interface doesn't support
> > to write node_states[], however write support is needed in case
> > users want to set them manually e.g. when user want to override
> > default N_DEMOTION_TARGETS found by the kernel.
> >
> > Rename existing _NODE_ATTR to _NODE_ATTR_RO and introduce new
> > _NODE_ATTR_RW which can be used for node_states[] which can
> > be written from sysfs.
> >
> > It may be necessary to validate written values and take action
> > based on them in a state specific way so a callback 'write' is
> > introduced in 'struct node_attr'.
> >
> > A new function demotion_targets_write() is added to validate
> > the input nodes for N_DEMOTION_TARGETS which should be subset
> > of N_MEMORY and to build new demotion list based on new nodes.
> >
> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > ---
> >  drivers/base/node.c | 62 +++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 49 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/base/node.c b/drivers/base/node.c
> > index 6eef22e6413e..e03eedbc421b 100644
> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/swap.h>
> >  #include <linux/slab.h>
> > +#include <linux/migrate.h>
> >  
> >  static struct bus_type node_subsys = {
> >  	.name = "node",
> > @@ -1013,6 +1014,7 @@ void unregister_one_node(int nid)
> >  struct node_attr {
> >  	struct device_attribute attr;
> >  	enum node_states state;
> > +	int (*write)(nodemask_t nodes);
> >  };
> >  
> >  static ssize_t show_node_state(struct device *dev,
> > @@ -1024,23 +1026,57 @@ static ssize_t show_node_state(struct device *dev,
> >  			  nodemask_pr_args(&node_states[na->state]));
> >  }
> >  
> > -#define _NODE_ATTR(name, state) \
> > -	{ __ATTR(name, 0444, show_node_state, NULL), state }
> > +static ssize_t store_node_state(struct device *s,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	nodemask_t nodes;
> > +	struct node_attr *na = container_of(attr, struct node_attr, attr);
> > +
> > +	if (nodelist_parse(buf, nodes))
> > +		return -EINVAL;
> > +
> > +	if (na->write) {
> > +		if (na->write(nodes))
> > +			return -EINVAL;
> > +	} else {
> > +		node_states[na->state] = nodes;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static int demotion_targets_write(nodemask_t nodes)
> > +{
> > +	if (nodes_subset(nodes, node_states[N_MEMORY])) {
> > +		node_states[N_DEMOTION_TARGETS] = nodes;
> > +		set_migration_target_nodes();
> > +		return 0;
> > +	}
> 
> Does this require locking to avoid upating
> node_states[N_DEMOTION_TARGETS] while a parallel
> set_migratiotn_target_nodes() is running. 

I think locking is needed if set_migratiotn_target_nodes() is called
form here because currently exclusion is provided by memory hotplug
events being single-threaded, but as this path is not hot-plug event,
separate lock is needed.

> > +
> > +	return -EINVAL;
> > +}
> > +
> > +#define _NODE_ATTR_RO(name, state) \
> > +	{ __ATTR(name, 0444, show_node_state, NULL), state, NULL }
> > +
> > +#define _NODE_ATTR_RW(name, state, write_fn) \
> > +	{ __ATTR(name, 0644, show_node_state, store_node_state), state, write_fn }
> >  
> >  static struct node_attr node_state_attr[] = {
> > -	[N_POSSIBLE] = _NODE_ATTR(possible, N_POSSIBLE),
> > -	[N_ONLINE] = _NODE_ATTR(online, N_ONLINE),
> > -	[N_NORMAL_MEMORY] = _NODE_ATTR(has_normal_memory, N_NORMAL_MEMORY),
> > +	[N_POSSIBLE] = _NODE_ATTR_RO(possible, N_POSSIBLE),
> > +	[N_ONLINE] = _NODE_ATTR_RO(online, N_ONLINE),
> > +	[N_NORMAL_MEMORY] = _NODE_ATTR_RO(has_normal_memory, N_NORMAL_MEMORY),
> >  #ifdef CONFIG_HIGHMEM
> > -	[N_HIGH_MEMORY] = _NODE_ATTR(has_high_memory, N_HIGH_MEMORY),
> > +	[N_HIGH_MEMORY] = _NODE_ATTR_RO(has_high_memory, N_HIGH_MEMORY),
> >  #endif
> > -	[N_MEMORY] = _NODE_ATTR(has_memory, N_MEMORY),
> > -	[N_CPU] = _NODE_ATTR(has_cpu, N_CPU),
> > -	[N_GENERIC_INITIATOR] = _NODE_ATTR(has_generic_initiator,
> > -					   N_GENERIC_INITIATOR),
> > -	[N_DEMOTION_TARGETS] = _NODE_ATTR(demotion_targets,
> > -					  N_DEMOTION_TARGETS),
> > -
> > +	[N_MEMORY] = _NODE_ATTR_RO(has_memory, N_MEMORY),
> > +	[N_CPU] = _NODE_ATTR_RO(has_cpu, N_CPU),
> > +	[N_GENERIC_INITIATOR] = _NODE_ATTR_RO(has_generic_initiator,
> > +					      N_GENERIC_INITIATOR),
> > +	[N_DEMOTION_TARGETS] = _NODE_ATTR_RW(demotion_targets,
> > +					     N_DEMOTION_TARGETS,
> > +					     demotion_targets_write),
> >  };
> >  
> >  static struct attribute *node_state_attrs[] = {
> > -- 
> > 2.35.1