Re: [PATCH v4 1/3] x86/sgx: Report SGX memory in /sys/devices/system/node/node*/meminfo

[Jarkko Sakkinen]

On Fri, 2021-09-10 at 17:11 +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 10, 2021 at 06:02:59PM +0300, Jarkko Sakkinen wrote:
> > On Fri, 2021-09-10 at 16:05 +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 10, 2021 at 04:17:44PM +0300, Jarkko Sakkinen wrote:
> > > > On Fri, 2021-09-10 at 08:51 +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Sep 10, 2021 at 03:17:24AM +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 SGX_MemTotal field to /sys/devices/system/node/node*/meminfo,
> > > > > > showing the total SGX memory in each NUMA node. The total memory for
> > > > > > each NUMA node is calculated by adding the sizes of contained EPC
> > > > > > sections together.
> > > > > > 
> > > > > > Introduce arch_node_read_meminfo(), which can optionally be rewritten by
> > > > > > the arch code, and rewrite it for x86 so it prints SGX_MemTotal.
> > > > > > 
> > > > > > Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > > > > > ---
> > > > > > v4:
> > > > > > * A new patch.
> > > > > >  arch/x86/kernel/cpu/sgx/main.c | 14 ++++++++++++++
> > > > > >  arch/x86/kernel/cpu/sgx/sgx.h  |  6 ++++++
> > > > > >  drivers/base/node.c            | 10 +++++++++-
> > > > > >  3 files changed, 29 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Where is the Documentation/ABI/ update for this new sysfs file?
> > > > 
> > > > It's has been existed for a long time, e.g.
> > > > 
> > > >  cat /sys/devices/system/node/node0/meminfo
> > > > Node 0 MemTotal:       32706792 kB
> > > > Node 0 MemFree:         5382988 kB
> > > > Node 0 MemUsed:        27323804 kB
> > > > Node 0 SwapCached:            8 kB
> > > > Node 0 Active:          3640612 kB
> > > > Node 0 Inactive:       21757684 kB
> > > > Node 0 Active(anon):    2833772 kB
> > > > Node 0 Inactive(anon):    14392 kB
> > > > Node 0 Active(file):     806840 kB
> > > > Node 0 Inactive(file): 21743292 kB
> > > > Node 0 Unevictable:       80640 kB
> > > > Node 0 Mlocked:           80640 kB
> > > > Node 0 Dirty:                36 kB
> > > > Node 0 Writeback:             0 kB
> > > > Node 0 FilePages:      22833124 kB
> > > > Node 0 Mapped:          1112832 kB
> > > > Node 0 AnonPages:       2645812 kB
> > > > Node 0 Shmem:            282984 kB
> > > > Node 0 KernelStack:       18544 kB
> > > > Node 0 PageTables:        46704 kB
> > > > Node 0 NFS_Unstable:          0 kB
> > > > Node 0 Bounce:                0 kB
> > > > Node 0 WritebackTmp:          0 kB
> > > > Node 0 KReclaimable:    1311812 kB
> > > > Node 0 Slab:            1542220 kB
> > > > Node 0 SReclaimable:    1311812 kB
> > > > Node 0 SUnreclaim:       230408 kB
> > > > Node 0 AnonHugePages:         0 kB
> > > > Node 0 ShmemHugePages:        0 kB
> > > > Node 0 ShmemPmdMapped:        0 kB
> > > > Node 0 FileHugePages:        0 kB
> > > > Node 0 FilePmdMapped:        0 kB
> > > > Node 0 HugePages_Total:     0
> > > > Node 0 HugePages_Free:      0
> > > > Node 0 HugePages_Surp:      0
> > 
> > This was something also spinned my head a bit, i.e. why hugepages fields
> > are not aligned correctly.
> > 
> > > > This file is undocumented but the fields seem to reflect what is in
> > > > /proc/meminfo, so I added additional patch for documentation:
> > > > 
> > > > https://lore.kernel.org/linux-sgx/20210910001726.811497-3-jarkko@xxxxxxxxxx/
> > > > 
> > > > I have no idea why things are how they are. I'm just merely trying to follow
> > > > the existing patterns. I'm also fully aware of the defacto sysfs pattern, i.e.
> > > > one value per file.
> > > 
> > > Then please do not add anything else to this nightmare of a mess.
> > 
> > There is already /sys/devices/system/node/node0/hugepages/.
> > 
> > It has alike data to the contents of meminfo:
> > 
> > /sys/devices/system/node/node0/hugepages/hugepages-2048kB:
> > surplus_hugepages
> > nr_hugepages
> > free_hugepages
> > 
> > /sys/devices/system/node/node0/hugepages/hugepages-1048576kB:
> > surplus_hugepages
> > nr_hugepages
> > free_hugepages
> > 
> > [I'm wondering tho, how the fields in meminfo are supposed to be interpreted,
> >  given that there are two types of huge pages, but let's just ignore that
> >  for the moment.]
> > 
> > Following this pattern, I'd perhaps go and create (and document):
> > 
> > /sys/devices/system/node/node0/sgx/memory_size
> > 
> > which would have the size in bytes.
> 
> If it is one-value-per-file, that's fine with me.

And if it's represented like this to user space, I think that /proc/meminfo
change is unnecessary: it's less involving (e.g. in scripting) to parse these
files in a loop and sum then, than grep the appropriate line from /proc/meminfo.
Would no make much common sense to maintain it.

For me the only open here is: would it make more sense to add x86
subdirectory to each node? With this approach the path would be
something like:

* /sys/devices/system/node/node0/x86/sgx_memory_size

I guess huge pages is different in the sense that it's not just one arch thing,
whereas SGX is directly arch related, so I think that this would be a more
civilized way to deal with SGX. This way things won't bloat, if or when something
else ought to need a NUMA node specific attribute.

> thanks,
> 
> greg k-h

/Jarkko