Messages in this thread | | | Date | Fri, 26 Oct 2018 16:42:25 -0400 | From | Sasha Levin <> | Subject | Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool |
| |
On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote: >This change was suggested by checkpath.pl. Use unsigned int with bitfield >allocate only one bit to the boolean variable. > >CHECK: Avoid using bool structure members because of possible alignment >issues > >Signed-off-by: Shayenne da Luz Moura <shayenneluzmoura@gmail.com> >--- > drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++------- > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +- > 2 files changed, 8 insertions(+), 8 deletions(-) > >diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h >index 594f84272957..7d3e329a6b1c 100644 >--- a/drivers/staging/vboxvideo/vbox_drv.h >+++ b/drivers/staging/vboxvideo/vbox_drv.h >@@ -81,7 +81,7 @@ struct vbox_private { > u8 __iomem *vbva_buffers; > struct gen_pool *guest_pool; > struct vbva_buf_ctx *vbva_info; >- bool any_pitch; >+ unsigned int any_pitch:1; > u32 num_crtcs; > /** Amount of available VRAM, including space used for buffers. */ > u32 full_vram_size;
Using bitfields for booleans in these cases is less efficient than just using "regular" booleans for two reasons:
1. It will use the same amount of space. Due to alignment requirements, the compiler can't squeeze in anything into the 7 bits that are now "free". Each member, unless it's another bitfield, must start at a whole byte.
2. This is actually less efficient (slower) for the compiler to work with. The smallest granularity we have to access memory is 1 byte; we can't set individual bits directly in memory. For the original code, the assembly for 'vbox_private.any_pitch = true' would look something like this:
movl $0x1,-0x10(%rsp)
As you can see, the compiler can directly write into the variable. However, when we switch to using bitfields, the compiler must preserve the original value of the other 7 bits, so it must first read them from memory, manipulate the value and write it back. The assembly would look something like this:
movzbl -0x10(%rsp),%eax or $0x1,%eax mov %al,-0x10(%rsp)
Which is less efficient than what was previously happening.
-- Thanks, Sasha
| |