| Message ID | 20230712230202.47929-4-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Cgroup support for SGX EPC memory | expand |
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
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
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
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?
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
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).
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 --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);