Re: [PATCH v3 03/28] x86/sgx: Add 'struct sgx_epc_lru_lists' to encapsulate lru list(s)

[Huang, Kai]

On Mon, 2023-07-24 at 09:55 -0500, Haitao Huang wrote:
> Hi Kai
> On Mon, 24 Jul 2023 05:04:48 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
> 
> > On Mon, 2023-07-17 at 08:23 -0500, Haitao Huang wrote:
> > > On Mon, 17 Jul 2023 07:45:36 -0500, Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > > wrote:
> > > 
> > > > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> > > > > From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> > > > > 
> > > > > Introduce a data structure to wrap the existing reclaimable list
> > > > > and its spinlock in a struct to minimize the code changes needed
> > > > > to handle multiple LRUs as well as reclaimable and non-reclaimable
> > > > > lists. The new structure will be used in a following set of patches  
> > > to
> > > > > implement SGX EPC cgroups.
> > 
> > Although briefly mentioned in the first patch, it would be better to put  
> > more
> > background about the "reclaimable" and "non-reclaimable" thing here,  
> > focusing on
> > _why_ we need multiple LRUs (presumably you mean two lists: reclaimable  
> > and non-
> > reclaimable).
> > 
> Sure I can add a little more background to introduce the  
> reclaimable/unreclaimable concept. But why we need multiple LRUs would be  
> self-evident in later patches, not sure I will add details here.

In this case people will need to go to that patch to get some idea first.  It
doesn't seem hurt if you can explain why you need multiple LRUs here first.

[...]

> > > > > +struct sgx_epc_lru_lists {
> > > > > +	/* Must acquire this lock to access */
> > > > > +	spinlock_t lock;
> > > > 
> > > > Isn't this self-explanatory, why the inline comment?
> > > 
> > > I got a warning from the checkpatch script complaining this lock needs
> > > comments.
> > 
> > I suspected this, so I applied this patch, removed the comment,  
> > generated a new
> > patch, and run checkpatch.pl for it.  It didn't report any warning/error  
> > in my
> > testing.
> > 
> > Are you sure you got a warning?
> 
> I did a reran and it's actually a "CHECK" I got:
> 
> $ ./scripts/checkpatch.pl --strict  
> 0001-x86-sgx-Add-struct-sgx_epc_lru_lists-to-encapsulate-.patch
> CHECK: spinlock_t definition without comment
> #41: FILE: arch/x86/kernel/cpu/sgx/sgx.h:101:
> +       spinlock_t lock;
> 
> total: 0 errors, 0 warnings, 1 checks, 22 lines checked
> 

I didn't get the CHECK in my testing.  Not sure why.

Anyway, I guess the comment can be useful if it is to explain why we need to use
spinlock or whatever lock.  But

	/* Must acquire this lock to access */

doesn't explain why at all, thus doesn't look helpful to me.

I guess you either need a better comment, or just remove it (it's obvious that a
lot of kernel code doesn't have a comment around spinlock_t).