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

[Kristen Carlson Accardi <kristen@linux.intel.com>]

On Fri, 2022-01-07 at 14:25 +0200, Jarkko Sakkinen wrote:
> 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

I can see your point. Since Boris objected to the module param, the
next version will not have a way to disable limits at all, so I am
deleting that boolean all together.