linux-kernel - Re: [PATCH v3 03/28] x86/sgx: Add 'struct sgx_epc_lru

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Jul 2023 23:31:58 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "mingo@...hat.com" <mingo@...hat.com>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "haitao.huang@...ux.intel.com" <haitao.huang@...ux.intel.com>,
        "tj@...nel.org" <tj@...nel.org>, "x86@...nel.org" <x86@...nel.org>
CC:     "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "Li, Zhiquan1" <zhiquan1.li@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v3 03/28] x86/sgx: Add 'struct sgx_epc_lru_lists' to
 encapsulate lru list(s)

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@...el.com> 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@...nel.org>
> > > wrote:
> > > 
> > > > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> > > > > From: Kristen Carlson Accardi <kristen@...ux.intel.com>
> > > > > 
> > > > > 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).