Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220422195516.10769-1-jvgediya@linux.ibm.com> <20220422195516.10769-4-jvgediya@linux.ibm.com>
In-Reply-To: <20220422195516.10769-4-jvgediya@linux.ibm.com>
From:   Wei Xu <weixugc@google.com>
Date:   Fri, 22 Apr 2022 13:32:29 -0700
Message-ID: <CAAPL-u8+LUrw-k_WKs2VESvFMzhNOFrG8X=E58Gvfw4FKvny9w@mail.gmail.com>
Subject: Re: [PATCH v3 3/7] drivers/base/node: Add support to write
 node_states[] via sysfs
To:     Jagdish Gediya <jvgediya@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,
        aneesh.kumar@linux.ibm.com, shy828301@gmail.com,
        gthelen@google.com, dan.j.williams@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 22, 2022 at 12:55 PM Jagdish Gediya <jvgediya@linux.ibm.com> wrote:
>
> 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>

Acked-by: Wei Xu <weixugc@google.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;
> +       }
> +
> +       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
>