On Fri, Jan 07, 2022 at 09:17:03AM -0800, Kristen Carlson Accardi wrote: > 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. Ok, cool, I'm looking forward for the next version. /Jarkko
