Re: [PATCHv5 04/10] node: Link memory nodes to their compute nodes

Rafael J. Wysocki
2019-02-05T12:33:27Z

On Fri, Jan 25, 2019 at 12:08 AM Keith Busch <keith.busch@intel.com> wrote:
>
> Systems may be constructed with various specialized nodes. Some nodes
> may provide memory, some provide compute devices that access and use
> that memory, and others may provide both. Nodes that provide memory are
> referred to as memory targets, and nodes that can initiate memory access
> are referred to as memory initiators.
>
> Memory targets will often have varying access characteristics from
> different initiators, and platforms may have ways to express those
> relationships. In preparation for these systems, provide interfaces for
> the kernel to export the memory relationship among different nodes memory
> targets and their initiators with symlinks to each other.
>
> If a system provides access locality for each initiator-target pair, nodes
> may be grouped into ranked access classes relative to other nodes. The
> new interface allows a subsystem to register relationships of varying
> classes if available and desired to be exported.
>
> A memory initiator may have multiple memory targets in the same access
> class. The target memory's initiators in a given class indicate the
> nodes access characteristics share the same performance relative to other
> linked initiator nodes. Each target within an initiator's access class,
> though, do not necessarily perform the same as each other.
>
> A memory target node may have multiple memory initiators. All linked
> initiators in a target's class have the same access characteristics to
> that target.
>
> The following example show the nodes' new sysfs hierarchy for a memory
> target node 'Y' with access class 0 from initiator node 'X':
>
> # symlinks -v /sys/devices/system/node/nodeX/access0/
> relative: /sys/devices/system/node/nodeX/access0/targets/nodeY -> ../../nodeY
>
> # symlinks -v /sys/devices/system/node/nodeY/access0/
> relative: /sys/devices/system/node/nodeY/access0/initiators/nodeX -> ../../nodeX
>
> The new attributes are added to the sysfs stable documentation.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 25 ++++-
> drivers/base/node.c | 142 +++++++++++++++++++++++++++-
> include/linux/node.h | 7 +-
> 3 files changed, 171 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index 3e90e1f3bf0a..fb843222a281 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -90,4 +90,27 @@ Date: December 2009
> Contact: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Description:
> The node's huge page size control/query attributes.
> - See Documentation/admin-guide/mm/hugetlbpage.rst
> \ No newline at end of file
> + See Documentation/admin-guide/mm/hugetlbpage.rst
> +
> +What: /sys/devices/system/node/nodeX/accessY/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The node's relationship to other nodes for access class "Y".
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing symlinks to memory initiator
> + nodes that have class "Y" access to this target node's
> + memory. CPUs and other memory initiators in nodes not in
> + the list accessing this node's memory may have different
> + performance.
> +
> +What: /sys/devices/system/node/nodeX/classY/targets/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing symlinks to memory targets that
> + this initiator node has class "Y" access.
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 86d6cd92ce3d..6f4097680580 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -17,6 +17,7 @@
> #include <linux/nodemask.h>
> #include <linux/cpu.h>
> #include <linux/device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/swap.h>
> #include <linux/slab.h>
>
> @@ -59,6 +60,94 @@ static inline ssize_t node_read_cpulist(struct device *dev,
> static DEVICE_ATTR(cpumap, S_IRUGO, node_read_cpumask, NULL);
> static DEVICE_ATTR(cpulist, S_IRUGO, node_read_cpulist, NULL);
>
> +/**
> + * struct node_access_nodes - Access class device to hold user visible
> + * relationships to other nodes.
> + * @dev: Device for this memory access class
> + * @list_node: List element in the node's access list
> + * @access: The access class rank
> + */
> +struct node_access_nodes {
> + struct device dev;

I'm not sure if the entire struct device is needed here.

It looks like what you need is the kobject part of it only and you can
use a kobject directly here:

struct kobject kobj;

Then, you can register that under the node's kobject using
kobject_init_and_add() and you can create attr groups under a kobject
using sysfs_create_groups(), which is exactly what device_add_groups()
does.

That would allow you to avoid allocating extra memory to hold the
entire device structure and the extra empty "power" subdirectory added
by device registration would not be there.

> + struct list_head list_node;
> + unsigned access;
> +};

Apart from the above, the patch looks good to me.