Re: [RFC][PATCH] bitfields API

[Vegard Nossum <vegard.nossum@gmail.com>]

On Thu, Aug 28, 2008 at 9:38 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On Thu, Aug 28, 2008 at 9:02 PM, Pekka J Enberg <penberg@cs.helsinki.fi> wrote:
>> Why not do something like this (as suggested by Ingo, I think)? Yeah, the
>> macro should go into kmemcheck.h but I don't have a tree handy...
>>
>>                Pekka
>>
>> Index: linux-2.6/include/linux/bitfield.h
>> ===================================================================
>> --- /dev/null
>> +++ linux-2.6/include/linux/bitfield.h
>> @@ -0,0 +1,10 @@
>> +#ifndef __LINUX_BITFIELD_H
>> +#define __LINUX_BITFIELD_H
>> +
>> +#ifdef CONFIG_KMEMCHECK
>> +#define KMEMCHECK_BIT_FIELD(field) do { field = 0; } while (0)
>> +#else
>> +#define KMEMCHECK_BIT_FIELD(field) do { } while (0)
>> +#endif /* CONFIG_KMEMCHECK */
>> +
>> +#endif /* __LINUX_BITFIELD_H */
>>
>> Index: linux-2.6/net/core/skbuff.c
>> ===================================================================
>> --- linux-2.6.orig/net/core/skbuff.c
>> +++ linux-2.6/net/core/skbuff.c
>> @@ -55,6 +55,7 @@
>>  #include <linux/rtnetlink.h>
>>  #include <linux/init.h>
>>  #include <linux/scatterlist.h>
>> +#include <linux/bitfield.h>
>>
>>  #include <net/protocol.h>
>>  #include <net/dst.h>
>> @@ -209,6 +210,11 @@ struct sk_buff *__alloc_skb(unsigned int
>>        skb->data = data;
>>        skb_reset_tail_pointer(skb);
>>        skb->end = skb->tail + size;
>> +       KMEMCHECK_BIT_FIELD(skb->local_df);
>> +       KMEMCHECK_BIT_FIELD(skb->cloned);
>> +       KMEMCHECK_BIT_FIELD(skb->ip_summed);
>> +       KMEMCHECK_BIT_FIELD(skb->nohdr);
>> +       KMEMCHECK_BIT_FIELD(skb->nfctinfo);
>>        /* make sure we initialize shinfo sequentially */
>>        shinfo = skb_shinfo(skb);
>>        atomic_set(&shinfo->dataref, 1);
>>
>
> That looks good to me. If the extra lines are okay with net people, we
> can put this in the fixlets branch and make it the norm for dealing
> with bitfields in kmemcheck.
>
> One thing to keep in mind that if the members of the bitfield does not
> span the entire width of the bitfield, the remaining bits must also be
> assigned (as an extra "filler" member), otherwise GCC will not
> optimize it to a single store. But that is not an issue in this
> particular case since all the bits are used.

Hm, and this is exactly the case for the "do_not_encrypt" field of skbuff:

#ifdef CONFIG_IPV6_NDISC_NODETYPE
        __u8                    ndisc_nodetype:2;
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
        __u8                    do_not_encrypt:1;
#endif  

So we have no way to know where or how big the filler should be. This is
why the simple patch above is not sufficient.

Alexey: I have a modified proposal with slightly different syntax for
DEFINE_BITFIELD. Can you say whether this is acceptable or not? Please
see this short mockup example:


<--- cut --->

#include <stdint.h>
#include <string.h>

#define DEFINE_BITFIELD(name, fields...)	\
	union {					\
		struct fields name;		\
		struct fields;			\
	};

#define KMEMCHECK_ANNOTATE_BITFIELD(bitfield)			\
	do {							\
		memset(&(bitfield), 0, sizeof(bitfield));	\
	} while(0)

struct skbuff {
	DEFINE_BITFIELD(flags1, {
		uint8_t			pkt_type:3,
					fclone:2,
					ipvs_property:1,
					peeked:1,
					nf_trace:1;
		uint16_t		protocol;
	});

	DEFINE_BITFIELD(flags2, {
#ifdef CONFIG_IPV6_NDISC_NODETYPE
		uint8_t			ndisc_nodetype:2;
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
		uint8_t			do_not_encrypt:1;
#endif
	});
};

void __alloc_skb(struct skbuff *skb)
{
	KMEMCHECK_ANNOTATE_BITFIELD(skb->flags1);
	KMEMCHECK_ANNOTATE_BITFIELD(skb->flags2);
}

<--- cut --->


Thanks,


Vegard