Messages in this thread | | | Subject | Re: [PATCH v6 2/2] x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node | From | Jarkko Sakkinen <> | Date | Tue, 28 Sep 2021 21:20:41 +0300 |
| |
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@linuxfoundation.org> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > > > 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@intel.com/ > > > > v5: > > * A new patch based on the discussion on > > https://lore.kernel.org/linux-sgx/3a7cab4115b4f902f3509ad8652e616b91703e1d.camel@kernel.org/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.
> > > + 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.
> thanks, > > greg k-h
/Jarkko
| |