Re: [PATCH v6 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node

[Jarkko Sakkinen]

On Wed, 2021-09-29 at 09:22 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 28, 2021 at 09:20:41PM +0300, Jarkko Sakkinen wrote:
> > On Tue, 2021-09-28 at 06:54 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Sep 28, 2021 at 06:13:50AM +0300, Jarkko Sakkinen wrote:
> > > > The amount of SGX memory on the system is determined by the BIOS and it
> > > > varies wildly between systems.  It can be from dozens of MB's on desktops
> > > > or VM's, up to many GB's on servers.  Just like for regular memory, it is
> > > > sometimes useful to know the amount of usable SGX memory in the system.
> > > > 
> > > > Add an attribute for the amount of SGX memory in bytes to each NUMA
> > > > node. The path is /sys/devices/system/node/node[0-9]*/sgx/memory_size.
> > > > Calculate these values by summing up EPC section sizes for each node
> > > > during the driver initalization.
> > > > 
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > > > ---
> > > > 
> > > > v6:
> > > > * Initialize node->size to zero in sgx_setup_epc_section(), when the
> > > >   node is first accessed. The bug report:
> > > >   https://lore.kernel.org/linux-sgx/f45245ba-41b8-62ae-38b5-64725a214bad@xxxxxxxxx/
> > > > 
> > > > v5:
> > > > * A new patch based on the discussion on
> > > >   https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@xxxxxxxxxx/T/#t
> > > > 
> > > >  Documentation/x86/sgx.rst      | 14 ++++++
> > > 
> > > sysfs files have to be documented in Documentation/ABI/ so that they can
> > > be automatically checked, and added to the documentation output
> > > properly.  Please do that here as well.
> > 
> > Right, I'll document it to sysfs-devices-node.
> > 
> > > >  arch/x86/kernel/cpu/sgx/main.c | 91 ++++++++++++++++++++++++++++++++++
> > > >  arch/x86/kernel/cpu/sgx/sgx.h  |  2 +
> > > >  3 files changed, 107 insertions(+)
> > > > 
> > > > diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
> > > > index dd0ac96ff9ef..f9d9cfa6dbf9 100644
> > > > --- a/Documentation/x86/sgx.rst
> > > > +++ b/Documentation/x86/sgx.rst
> > > > @@ -250,3 +250,17 @@ user wants to deploy SGX applications both on the host and in guests
> > > >  on the same machine, the user should reserve enough EPC (by taking out
> > > >  total virtual EPC size of all SGX VMs from the physical EPC size) for
> > > >  host SGX applications so they can run with acceptable performance.
> > > > +
> > > > +Per NUMA node SGX attributes
> > > > +============================
> > > > +
> > > > +NUMA nodes devices expose SGX specific attributes in the following path:
> > > > +
> > > > +	/sys/devices/system/node/node[0-9]*/sgx/
> > > > +
> > > > +Attributes
> > > > +----------
> > > > +
> > > > +memory_size
> > > > +                Total available physical SGX memory, also known as Enclave
> > > > +                Page Cache (EPC), in bytes.
> > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > > > index a6e313f1a82d..4f1e3b5e3d14 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -714,9 +714,11 @@ static bool __init sgx_page_cache_init(void)
> > > >  			spin_lock_init(&sgx_numa_nodes[nid].lock);
> > > >  			INIT_LIST_HEAD(&sgx_numa_nodes[nid].free_page_list);
> > > >  			node_set(nid, sgx_numa_mask);
> > > > +			sgx_numa_nodes[nid].size = 0;
> > > >  		}
> > > >  
> > > >  		sgx_epc_sections[i].node =  &sgx_numa_nodes[nid];
> > > > +		sgx_numa_nodes[nid].size += size;
> > > >  
> > > >  		sgx_nr_epc_sections++;
> > > >  	}
> > > > @@ -790,6 +792,87 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> > > >  
> > > > +#ifdef CONFIG_NUMA
> > > > +static void sgx_numa_exit(void)
> > > > +{
> > > > +	int nid;
> > > > +
> > > > +	for (nid = 0; nid < num_possible_nodes(); nid++) {
> > > > +		if (!sgx_numa_nodes[nid].kobj)
> > > > +			continue;
> > > > +
> > > > +		kobject_put(sgx_numa_nodes[nid].kobj);
> > > > +	}
> > > > +}
> > > > +
> > > > +#define SGX_NODE_ATTR_RO(_name) \
> > > > +	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> > > 
> > > Why are you dealing with a "raw" kobject?  Shouldn't you have a device
> > > and use a device attribute?
> > > 
> > > > +static bool sgx_numa_init(void)
> > > > +{
> > > > +	struct sgx_numa_node *node;
> > > > +	struct device *dev;
> > > > +	int nid;
> > > > +	int ret;
> > > > +
> > > > +	for (nid = 0; nid < num_possible_nodes(); nid++) {
> > > > +		if (!sgx_numa_nodes[nid].size)
> > > > +			continue;
> > > > +
> > > > +		node = &sgx_numa_nodes[nid];
> > > > +		dev = &node_devices[nid]->dev;
> > > > +
> > > > +		node->kobj = kobject_create_and_add("sgx", &dev->kobj);
> > > 
> > > You just "broke" the tree by putting a raw kobject below a struct
> > > device.  Please do not do that.
> > 
> > I looked how hugetlb was implemented as a starting point, since it is
> > existing mainline code, i.e. in mm/hugetlb.c:
> > 
> > - hugetlb_register_node()
> > - hugetlb_register_all_nodes()
> > - HSTATE_ATTR_RO()
> > 
> > hugetlb code attaches raw kobjects to the node device, by using this
> > (perhaps anti)pattern.
> 
> Never look at the memory code for how to use sysfs "properly" :)
> 
> It deals with raw kobjects as it is not tied into the driver model at
> all.  You are, so do not try to route around it, userspace will not be
> happy with you if you do so.
> 
> > > > +		if (!node->kobj) {
> > > > +			sgx_numa_exit();
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		ret = sysfs_create_group(node->kobj, &sgx_node_attr_group);
> > > 
> > > And you raced with userspace and lost.
> > > 
> > > Wait, you have a kobject _just_ for a subdirectory name?  Why?  Use a
> > > named attribute group, that's exactly what that is for.
> > > 
> > > Properly attach your attributes to the device you have, don't do extra
> > > work and complex code that you do not have to at all.
> > 
> > Here the reference was hugetlb_sysfs_init() and hugetlb_sysfs_add_hstate().
> > Agreed, that a named group would make a lot more sense.
> 
> Again, ignore all sysfs code for the memory subsystem please :)

OK, cool, thank you. I can do that.

> 
> thanks,
> 
> greg k-h

/Jarkko