diff mbox series

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

Message ID 20230712230202.47929-4-haitao.huang@linux.intel.com
State New
Headers
Series Add Cgroup support for SGX EPC memory |

Commit Message

Haitao Huang July 12, 2023, 11:01 p.m. UTC 
  From: Kristen Carlson Accardi <kristen@linux.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.

The changes to the structure needed for unreclaimable lists will be
added in later patches.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Cc: Sean Christopherson <seanjc@google.com>

V3:
Removed the helper functions and revised commit messages
---
 arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Jarkko Sakkinen July 17, 2023, 12:45 p.m. UTC | #1 
On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@linux.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.
>
> The changes to the structure needed for unreclaimable lists will be
> added in later patches.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Cc: Sean Christopherson <seanjc@google.com>
>
> V3:
> Removed the helper functions and revised commit messages
> ---
>  arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index f6e3c5810eef..77fceba73a25 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -92,6 +92,23 @@ static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page)
>  	return section->virt_addr + index * PAGE_SIZE;
>  }
>  
> +/*
> + * This data structure wraps a list of reclaimable EPC pages, and a list of
> + * non-reclaimable EPC pages and is used to implement a LRU policy during
> + * reclamation.
> + */
> +struct sgx_epc_lru_lists {
> +	/* Must acquire this lock to access */
> +	spinlock_t lock;

Isn't this self-explanatory, why the inline comment?

> +	struct list_head reclaimable;
> +};
> +
> +static inline void sgx_lru_init(struct sgx_epc_lru_lists *lrus)
> +{
> +	spin_lock_init(&lrus->lock);
> +	INIT_LIST_HEAD(&lrus->reclaimable);
> +}
> +
>  struct sgx_epc_page *__sgx_alloc_epc_page(void);
>  void sgx_free_epc_page(struct sgx_epc_page *page);
>  
> -- 
> 2.25.1


BR, Jarkko
Haitao Huang July 17, 2023, 1:23 p.m. UTC | #2 
On Mon, 17 Jul 2023 07:45:36 -0500, Jarkko Sakkinen <jarkko@kernel.org>  
wrote:

> On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
>> From: Kristen Carlson Accardi <kristen@linux.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.
>>
>> The changes to the structure needed for unreclaimable lists will be
>> added in later patches.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
>> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
>> Cc: Sean Christopherson <seanjc@google.com>
>>
>> V3:
>> Removed the helper functions and revised commit messages
>> ---
>>  arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
>> b/arch/x86/kernel/cpu/sgx/sgx.h
>> index f6e3c5810eef..77fceba73a25 100644
>> --- a/arch/x86/kernel/cpu/sgx/sgx.h
>> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
>> @@ -92,6 +92,23 @@ static inline void *sgx_get_epc_virt_addr(struct  
>> sgx_epc_page *page)
>>  	return section->virt_addr + index * PAGE_SIZE;
>>  }
>>
>> +/*
>> + * This data structure wraps a list of reclaimable EPC pages, and a  
>> list of
>> + * non-reclaimable EPC pages and is used to implement a LRU policy  
>> during
>> + * reclamation.
>> + */
>> +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.

Thanks
Haitao
Jarkko Sakkinen July 17, 2023, 2:39 p.m. UTC | #3 
On Mon Jul 17, 2023 at 1:23 PM UTC, Haitao Huang wrote:
> On Mon, 17 Jul 2023 07:45:36 -0500, Jarkko Sakkinen <jarkko@kernel.org>  
> wrote:
>
> > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> >> From: Kristen Carlson Accardi <kristen@linux.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.
> >>
> >> The changes to the structure needed for unreclaimable lists will be
> >> added in later patches.
> >>
> >> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> >> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> >> Cc: Sean Christopherson <seanjc@google.com>
> >>
> >> V3:
> >> Removed the helper functions and revised commit messages
> >> ---
> >>  arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
> >> b/arch/x86/kernel/cpu/sgx/sgx.h
> >> index f6e3c5810eef..77fceba73a25 100644
> >> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> >> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> >> @@ -92,6 +92,23 @@ static inline void *sgx_get_epc_virt_addr(struct  
> >> sgx_epc_page *page)
> >>  	return section->virt_addr + index * PAGE_SIZE;
> >>  }
> >>
> >> +/*
> >> + * This data structure wraps a list of reclaimable EPC pages, and a  
> >> list of
> >> + * non-reclaimable EPC pages and is used to implement a LRU policy  
> >> during
> >> + * reclamation.
> >> + */
> >> +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.

OK, cool, not a big deal.

BR, Jarkko
Kai Huang July 24, 2023, 10:04 a.m. UTC | #4 
On Mon, 2023-07-17 at 08:23 -0500, Haitao Huang wrote:
> On Mon, 17 Jul 2023 07:45:36 -0500, Jarkko Sakkinen <jarkko@kernel.org>  
> wrote:
> 
> > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> > > From: Kristen Carlson Accardi <kristen@linux.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).

> > > 
> > > The changes to the structure needed for unreclaimable lists will be
> > > added in later patches.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> > > Cc: Sean Christopherson <seanjc@google.com>
> > > 
> > > V3:
> > > Removed the helper functions and revised commit messages

Please put change history into:

---
  change history
---

So it can be stripped away when applying the patch.

> > > ---
> > >  arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
> > > b/arch/x86/kernel/cpu/sgx/sgx.h
> > > index f6e3c5810eef..77fceba73a25 100644
> > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > @@ -92,6 +92,23 @@ static inline void *sgx_get_epc_virt_addr(struct  
> > > sgx_epc_page *page)
> > >  	return section->virt_addr + index * PAGE_SIZE;
> > >  }
> > > 
> > > +/*
> > > + * This data structure wraps a list of reclaimable EPC pages, and a  
> > > list of
> > > + * non-reclaimable EPC pages and is used to implement a LRU policy  
> > > during
> > > + * reclamation.
> > > + */

I'd prefer to not mention the "non-reclaimable" thing in this patch, but defer
to the one actually introduces the "non-reclaimable" list.  Actually, I don't
think we even need this comment, given you have this in the structure:

	struct list_head reclaimable;

Which already explains the "reclaimable" list.  I suppose the non-reclaimable
list would be named similarly thus need no comment either.

Also, I am wondering why you need to split this out as a separate patch.  It
basically does nothing.  To me you should just merge this to the next patch,
which actually does what you claimed in the changelog:

	Introduce a data structure to wrap the existing reclaimable list and 
	its spinlock ...

Then this can be an infrastructure change patch, which doesn't bring any
functional change, to support the non-reclaimable list.


> > > +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?
Haitao Huang July 24, 2023, 2:55 p.m. UTC | #5 
Hi Kai
On Mon, 24 Jul 2023 05:04:48 -0500, Huang, Kai <kai.huang@intel.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@kernel.org>
>> wrote:
>>
>> > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
>> > > From: Kristen Carlson Accardi <kristen@linux.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.

>> > >
>> > > The changes to the structure needed for unreclaimable lists will be
>> > > added in later patches.
>> > >
>> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
>> > > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
>> > > Cc: Sean Christopherson <seanjc@google.com>
>> > >
>> > > V3:
>> > > Removed the helper functions and revised commit messages
>
> Please put change history into:
>
> ---
>   change history
> ---
>
> So it can be stripped away when applying the patch.
>
Will do that.

>> > > ---
>> > >  arch/x86/kernel/cpu/sgx/sgx.h | 17 +++++++++++++++++
>> > >  1 file changed, 17 insertions(+)
>> > >
>> > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
>> > > b/arch/x86/kernel/cpu/sgx/sgx.h
>> > > index f6e3c5810eef..77fceba73a25 100644
>> > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
>> > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
>> > > @@ -92,6 +92,23 @@ static inline void *sgx_get_epc_virt_addr(struct
>> > > sgx_epc_page *page)
>> > >  	return section->virt_addr + index * PAGE_SIZE;
>> > >  }
>> > >
>> > > +/*
>> > > + * This data structure wraps a list of reclaimable EPC pages, and a
>> > > list of
>> > > + * non-reclaimable EPC pages and is used to implement a LRU policy
>> > > during
>> > > + * reclamation.
>> > > + */
>
> I'd prefer to not mention the "non-reclaimable" thing in this patch, but  
> defer
> to the one actually introduces the "non-reclaimable" list.  Actually, I  
> don't
> think we even need this comment, given you have this in the structure:
>
> 	struct list_head reclaimable;
>

Agreed.

> Which already explains the "reclaimable" list.  I suppose the  
> non-reclaimable
> list would be named similarly thus need no comment either.
>
> Also, I am wondering why you need to split this out as a separate  
> patch.  It
> basically does nothing.  To me you should just merge this to the next  
> patch,

I think Kristen splitted the original patch based on Dave's comments:

https://lore.kernel.org/all/e71d76b2-4368-4627-abd4-2163e6786a20@intel.com/

> which actually does what you claimed in the changelog:
>
> 	Introduce a data structure to wrap the existing reclaimable list and 
> 	its spinlock ...
>
> Then this can be an infrastructure change patch, which doesn't bring any
> functional change, to support the non-reclaimable list.
>
>
>> > > +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

Thanks
Haitao
Kai Huang July 24, 2023, 11:31 p.m. UTC | #6 
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@intel.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@kernel.org>
> > > wrote:
> > > 
> > > > On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> > > > > From: Kristen Carlson Accardi <kristen@linux.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).
Haitao Huang July 31, 2023, 8:35 p.m. UTC | #7 
On Mon, 24 Jul 2023 18:31:58 -0500, Huang, Kai <kai.huang@intel.com> wrote:

...
>> > 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.
>
Will add.

...
>
> 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).
>

I'll remove the comments.
Thanks
Haitao
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index f6e3c5810eef..77fceba73a25 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -92,6 +92,23 @@  static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page)
 	return section->virt_addr + index * PAGE_SIZE;
 }
 
+/*
+ * This data structure wraps a list of reclaimable EPC pages, and a list of
+ * non-reclaimable EPC pages and is used to implement a LRU policy during
+ * reclamation.
+ */
+struct sgx_epc_lru_lists {
+	/* Must acquire this lock to access */
+	spinlock_t lock;
+	struct list_head reclaimable;
+};
+
+static inline void sgx_lru_init(struct sgx_epc_lru_lists *lrus)
+{
+	spin_lock_init(&lrus->lock);
+	INIT_LIST_HEAD(&lrus->reclaimable);
+}
+
 struct sgx_epc_page *__sgx_alloc_epc_page(void);
 void sgx_free_epc_page(struct sgx_epc_page *page);