[intel-sgx-kernel-dev] intel_sgx: compress sgx_encl_page flags and va_offset
diff mbox

Message ID 1492014258-19043-1-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson April 12, 2017, 4:24 p.m. UTC
Compress struct sgx_encl_page's flags and va_offset into a single
32-bit variable by assigning 20 bits of an unsigned int for flags
and the remaining 12 bits for the VA offset.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx/sgx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Sean Christopherson April 12, 2017, 5:18 p.m. UTC | #1
On Wed, 2017-04-12 at 09:24 -0700, Sean Christopherson wrote:
> Compress struct sgx_encl_page's flags and va_offset into a single
> 32-bit variable by assigning 20 bits of an unsigned int for flags
> and the remaining 12 bits for the VA offset.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

A few notes on this patch...

On a 64-bit system, simply moving va_offset to be adjacent to flags would be sufficient to reduce the memory footprint by 8 bytes, as that would eliminate the padding that is currently added to both flags and va_offset.  That being said, compressing the fields does save 4 bytes on a 32-bit system and provides the option for a "free" 32-bit variable (on 64-bit systems) in the future.

If desired, va_offset can be shrunk to 9 bits by tracking the slot instead of the offset, which would yield space for 3 more flags.  I opted not to implement that change as there are plenty of unused flags at this time, and working with the offset is more convenient and intuitive for the vast majority of the code.

> ---
>  drivers/platform/x86/intel_sgx/sgx.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h
> b/drivers/platform/x86/intel_sgx/sgx.h
> index 82355bb..2b7a7f2 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -105,11 +105,11 @@ enum sgx_encl_page_flags {
>  
>  struct sgx_encl_page {
>  	unsigned long addr;
> -	unsigned int flags;
> +	unsigned int flags : 20,
> +		     va_offset : 12;
>  	struct sgx_epc_page *epc_page;
>  	struct list_head load_list;
>  	struct sgx_va_page *va_page;
> -	unsigned int va_offset;
>  };
>  
>  struct sgx_tgid_ctx {
Jarkko Sakkinen April 12, 2017, 8:57 p.m. UTC | #2
On Wed, Apr 12, 2017 at 09:24:18AM -0700, Sean Christopherson wrote:
> Compress struct sgx_encl_page's flags and va_offset into a single
> 32-bit variable by assigning 20 bits of an unsigned int for flags
> and the remaining 12 bits for the VA offset.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

You never should use bitfields as the C standard does not give much
guarantees how they are compiled.

I would also probably mainly for cosmetic reasons suggest to use
explicit size type u32 for the field. It just makes the code a bit
more readable later on.

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx/sgx.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> index 82355bb..2b7a7f2 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -105,11 +105,11 @@ enum sgx_encl_page_flags {
>  
>  struct sgx_encl_page {
>  	unsigned long addr;
> -	unsigned int flags;
> +	unsigned int flags : 20,
> +		     va_offset : 12;
>  	struct sgx_epc_page *epc_page;
>  	struct list_head load_list;
>  	struct sgx_va_page *va_page;
> -	unsigned int va_offset;
>  };
>  
>  struct sgx_tgid_ctx {
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Sean Christopherson April 20, 2017, 4:27 p.m. UTC | #3
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> On Wed, Apr 12, 2017 at 09:24:18AM -0700, Sean Christopherson wrote:
> > Compress struct sgx_encl_page's flags and va_offset into a single
> > 32-bit variable by assigning 20 bits of an unsigned int for flags
> > and the remaining 12 bits for the VA offset.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> You never should use bitfields as the C standard does not give much
> guarantees how they are compiled.
> 
> I would also probably mainly for cosmetic reasons suggest to use
> explicit size type u32 for the field. It just makes the code a bit
> more readable later on.
> 
> /Jarkko

Bitfields certainly have their pitfalls and in most instances I agree
that they do more harm than good, but in this case we don't care about
the ordering of the fields and it's unlikely that future code will be
passing pointer to either field.  That being said, I'm not gung-ho for
adding a bitfield so I'm completely ok rejecting this patch.

> 
> > ---
> >  drivers/platform/x86/intel_sgx/sgx.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx/sgx.h
> > b/drivers/platform/x86/intel_sgx/sgx.h index 82355bb..2b7a7f2 100644
> > --- a/drivers/platform/x86/intel_sgx/sgx.h
> > +++ b/drivers/platform/x86/intel_sgx/sgx.h
> > @@ -105,11 +105,11 @@ enum sgx_encl_page_flags {
> >  
> >  struct sgx_encl_page {
> >     unsigned long addr;
> > -   unsigned int flags;
> > +   unsigned int flags : 20,
> > +            va_offset : 12;
> >     struct sgx_epc_page *epc_page;
> >     struct list_head load_list;
> >     struct sgx_va_page *va_page;
> > -   unsigned int va_offset;
> >  };
> >  
> >  struct sgx_tgid_ctx {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > intel-sgx-kernel-dev mailing list
> > intel-sgx-kernel-dev@lists.01.org
> > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Jarkko Sakkinen April 23, 2017, 12:16 p.m. UTC | #4
On Thu, Apr 20, 2017 at 04:27:26PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > On Wed, Apr 12, 2017 at 09:24:18AM -0700, Sean Christopherson wrote:
> > > Compress struct sgx_encl_page's flags and va_offset into a single
> > > 32-bit variable by assigning 20 bits of an unsigned int for flags
> > > and the remaining 12 bits for the VA offset.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > You never should use bitfields as the C standard does not give much
> > guarantees how they are compiled.
> > 
> > I would also probably mainly for cosmetic reasons suggest to use
> > explicit size type u32 for the field. It just makes the code a bit
> > more readable later on.
> > 
> > /Jarkko
> 
> Bitfields certainly have their pitfalls and in most instances I agree
> that they do more harm than good, but in this case we don't care about
> the ordering of the fields and it's unlikely that future code will be
> passing pointer to either field.  That being said, I'm not gung-ho for
> adding a bitfield so I'm completely ok rejecting this patch.

There's this thing that upstream developers hate them usually. Even
if you have case for them this would be a slowdown and people would
focus on unimportant detail.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
index 82355bb..2b7a7f2 100644
--- a/drivers/platform/x86/intel_sgx/sgx.h
+++ b/drivers/platform/x86/intel_sgx/sgx.h
@@ -105,11 +105,11 @@  enum sgx_encl_page_flags {
 
 struct sgx_encl_page {
 	unsigned long addr;
-	unsigned int flags;
+	unsigned int flags : 20,
+		     va_offset : 12;
 	struct sgx_epc_page *epc_page;
 	struct list_head load_list;
 	struct sgx_va_page *va_page;
-	unsigned int va_offset;
 };
 
 struct sgx_tgid_ctx {