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
