Re: [PATCH 1/2] x86/sgx: Add accounting for tracking overcommit

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Jan 06, 2022 at 10:26:18AM -0800, Kristen Carlson Accardi wrote:
> Hi Jarkko, thanks for your review,
> 
> On Wed, 2021-12-29 at 01:04 +0200, Jarkko Sakkinen wrote:
> > On Mon, Dec 20, 2021 at 09:46:39AM -0800, Kristen Carlson Accardi
> > wrote:
> > > 
> > > +
> > > +/**
> > > + * sgx_charge_mem() - charge for a page used for backing storage
> > > + *
> > 
> > Please remove this empty line:
> > 
> > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> 
> I read this to be that there should be an empty line after the short
> description/arg list but before the longer description. I think for
> functions without args this is the proper layout. It also is more
> readable.
> 
> > 
> > > + * Backing storage usage is capped by the
> > > sgx_nr_available_backing_pages.
> > > + * If the backing storage usage is over the overcommit limit,
> > 
> > Where does this verb "charge" come from?
> 
> Charge in this context means that some available backing pages are now
> not available because they are in use. It feels appropriate to me to
> use "charge/uncharge" verbs for this action, unless you think it's
> confusing somehow.

OK, it's cool.

I'm still wondering why you need extra variable given that
sgx_nr_backing_available_pages is signed. You could mark the
feature being disabled by setting it to -1.

It might cosmetically improve readability to have a boolean but
for practical uses (e.g. eBPF tracing scripts) it only adds extra
complexity.

/Jarkko