linux-kernel - Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1810280639150.2450@hadrien>
Date:   Sun, 28 Oct 2018 06:41:22 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Joe Perches <joe@...ches.com>
cc:     Julia Lawall <julia.lawall@...6.fr>,
        Sasha Levin <sashal@...nel.org>,
        Shayenne da Luz Moura <shayenneluzmoura@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Michael Thayer <michael.thayer@...cle.com>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        outreachy-kernel@...glegroups.com
Subject: Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use
 unsigned int instead bool



On Sat, 27 Oct 2018, Joe Perches wrote:

> On Fri, 2018-10-26 at 22:54 +0200, Julia Lawall wrote:
> > [Adding Joe Perches]
> >
> > On Fri, 26 Oct 2018, Sasha Levin wrote:
> >
> > > 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@...il.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.

Since this is between a pointer and a u32, won't the compiler put a lot
more padding, in both cases?

> > >
> > > 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.
> >
> > Maybe checkpatch could be more precise about what kind of bools should be
> > changed?
>
> Probably so, what verbiage would you suggest?

I don't know what are the conditions.  Sasha?

julia

> Also, any conversion from bool to int would
> have to take care than any assigment uses !!
> where appropriate.
>
>
>