| Message ID | 1492014258-19043-1-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 {
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
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
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
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 {
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(-)