gmane.linux.kernel.janitors

Re: [PATCH] lttng: cleanup one-bit signed bitfields

Mathieu Desnoyers — 2011-12-01T20:21:31

OK, will do next time, sorry,

Thanks!

Mathieu

Re: [PATCH] lttng: cleanup one-bit signed bitfields

Greg KH — 2011-12-01T17:56:06

Ick, I can edit this up, but please, make it in a format that I can
apply it in (i.e. git send-mail output).

thanks,

greg k-h

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

Mathieu Desnoyers — 2011-12-01T15:15:42

As we add more than one of those "flags", not using bitfields will grow
the memory footprint of these structures, so I don't think this is
advised here.

Thanks,

Mathieu

* walter harms (wharms< at >bfs.de) wrote:

Re: [PATCH] lttng: cleanup one-bit signed bitfields

Dan Carpenter — 2011-12-01T14:43:51

Acked-by: Dan Carpenter <dan.carpenter< at >oracle.com>

regards,
dan carpenter

_______________________________________________
devel mailing list
devel< at >linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

walter harms — 2011-12-01T14:41:23

Hello Mathieu,
nice to hear someone is concerned about space.
since you plan to go for uint perhaps we can drop that bitfield stuff at all ?

re,
 wh


Am 01.12.2011 15:20, schrieb Mathieu Desnoyers:

[PATCH] lttng: cleanup one-bit signed bitfields

Mathieu Desnoyers — 2011-12-01T14:31:18

commit 93db362daaaef88fed672256fa2948cf0efc11fb
Author: Mathieu Desnoyers <mathieu.desnoyers< at >efficios.com>
Date:   Thu Dec 1 09:25:40 2011 -0500

    lttng: cleanup one-bit signed bitfields
    
    * Dan Carpenter <dan.carpenter< at >oracle.com> wrote:
    > Sparse complains that these signed bitfields look "dubious".  The
    > problem is that instead of being either 0 or 1 like people would expect,
    > signed one bit variables like this are either 0 or -1.  It doesn't cause
    > a problem in this case but it's ugly so lets fix them.
    
    * walter harms (wharms< at >bfs.de) wrote:
    > hi,
    > This patch looks ok to me but this design is ugly by itself.
    > It should be replaced by an uchar uint whatever or use a
    > real bool (obviously not preferred by this programmes).
    
    bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
    any space here, because the surrounding fields are either uint or
    pointers, so alignment will just add padding.
    
    I try to use int/uint w

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

Mathieu Desnoyers — 2011-12-01T14:20:58

bool :1, uchar :1 or uint :1 could make sense. uchar:1/bool:1 won't save
any space here, because the surrounding fields are either uint or
pointers, so alignment will just add padding.

I try to use int/uint whenever possible because x86 CPUs tend to get
less register false-dependencies when using instructions modifying the
whole register (generated by using int/uint types) rather than only part
of it (uchar/char/bool). I only use char/uchar/bool when there is a
clear wanted space gain.

The reason why I never use the bool type within a structure when I want
a compact representation is that bool takes a whole byte just to
represent one bit:

#include <stdio.h>
#include <stdbool.h>

struct usebitfield {
        int a;
        unsigned int f:1, g:1, h:1, i:1, j:1;
        int b;
};

struct usebool {
        int a;
        bool f, g, h, i, j;
        int b;
};

struct useboolbf {
        int a;
        bool f:1, g:1, h:1, i:1, j:1;
        int b;
};

int main()
{
        printf("bitfield %d bytes, bool %d byte

Re: [patch] Staging: lttng: dubious one-bit signed bitfields

walter harms — 2011-12-01T09:56:01

hi,
This patch looks ok to me but this design is ugly by itself.
It should be replaced by an uchar uint whatever or use a
real bool (obviously not preferred by this programmes).

re,
 wh

Am 01.12.2011 10:37, schrieb Dan Carpenter:

[patch] Staging: lttng: dubious one-bit signed bitfields

Dan Carpenter — 2011-12-01T09:37:47

Sparse complains that these signed bitfields look "dubious".  The
problem is that instead of being either 0 or 1 like people would expect,
signed one bit variables like this are either 0 or -1.  It doesn't cause
a problem in this case but it's ugly so lets fix them.

Signed-off-by: Dan Carpenter <dan.carpenter< at >oracle.com>
---
I just did this against linux next but it applies fine on top of
Mathieu's recent patches.

diff --git a/drivers/staging/lttng/lib/ringbuffer/backend_types.h b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
index 1d301de..019929a 100644
--- a/drivers/staging/lttng/lib/ringbuffer/backend_types.h
+++ b/drivers/staging/lttng/lib/ringbuffer/backend_types.h
< at >< at > -65,7 +65,7 < at >< at > struct channel_backend {
  * for writer.
  */
 unsigned int buf_size_order;/* Order of buffer size */
-int extra_reader_sb:1;/* Bool: has extra reader subbuffer */
+unsigned int extra_reader_sb:1;/* Bool: has extra reader subbuffer */
 struct lib_ring_buffer *buf;/* Channel per-cpu buffers */