lkml.org 
[lkml]   [2008]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC][PATCH] bitfields API
From
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


\
 
 \ /
  Last update: 2008-08-30 10:31    [W:0.083 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site