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.
