Re: [PATCH v2 net-next] net: filter: export pkt_type_offset() helper

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

On Do, 2014-09-04 at 06:09 -0700, Eric Dumazet wrote:
> On Thu, 2014-09-04 at 13:50 +0200, Hannes Frederic Sowa wrote:
> 
> > 
> > Denis, I thought about something like this, you can incorperate this in
> > your patch if you can give it a test and check for other architectures,
> > thanks!
> > 
> 
> 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 02529fc..87b86aa 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -548,6 +548,10 @@ struct sk_buff {
> >  				ip_summed:2,
> >  				nohdr:1,
> >  				nfctinfo:3;
> > +	/* __pkt_type_offset marks the offset of the bitfield pkt_type
> > +	 * contains, so bpf can relatively address it
> > +	 */
> > +	int			__pkt_type_offset[0];
> 
> Why is it 'int', while the following uses __u8 ?

The type does not matter at all. Actually I wanted to use an empty
struct but was afraid it might not work on older compilers and didn't
want to check that with each version.

> This means pkt_type has to stay at this bit offset, and nothing will
> detect at compile time or execution time if someone did a reorg or added
> a new field (it seems to be the trend lately)
> 
> __u8                          bar:4,
>                               pkt_type:3,
> ...

That would be fine, one just has to adapt the PKT_TYPE_MAX macros in
case on reorders, __pkt_type_offset only needs to stay just in front of
the bitfield pkt_type is located in. I think we should move them up into
struct sk_buff, so they belong together and people see that they need to
change those in case they reorder fields.

I still think about using anonymous unions, they might let us compute
the pkt_type mask at compiletime, I think.

Also, maybe we can compute offset at compiletime if we switch to
anonymous union.

static inline unsigned int skb_pkt_type_offset(void)
{
 unsigned int mask;
 struct sk_buff skb = {.pkt_type = ~0U};
 mask = skb.__flags2;
 BUILD_BUG_ON(!__builtin_constant_p(mask));
 return mask;
}

something like that, haven't checked if that works.

> 
> 
> 
> >  	__u8			pkt_type:3,
> >  				fclone:2,
> >  				ipvs_property:1,
> > @@ -647,6 +651,17 @@ struct sk_buff {
> >  #define SKB_ALLOC_FCLONE	0x01
> >  #define SKB_ALLOC_RX		0x02
> >  
> 
> > +#ifdef __BIG_ENDIAN_BITFIELD
> > +#define PKT_TYPE_MAX   (7 << 5)
> > +#else
> > +#define PKT_TYPE_MAX   7
> > +#endif
> 
> It had a sense right before static int pkt_type_offset(void), but here,
> a casual reader might be lost.

Do you think we should just move PKT_TYPE_MAX macros right to the
definition of __pkt_type_offset?

> I am saying this because a reorder of the bit fields is probably needed
> to speedup __copy_skb_header() : Grouping together bit fields could
> allow optimized word copies instead of bit field manipulations.

Yes, I agree with you. It should be obvious to adapt the macros in case
someones reorders sk_buff. Either BUILD_BUG_ON or very obvious comments
+ declarations.

Thanks,
Hannes