RFC: on [ab]use of skb->cb by VLAN code

RFC: on [ab]use of skb->cb by VLAN code

[jamal]

I was going to forget this, but its been playing in the back of my head
and wont go away....
Matt Carlson recently (while fixing the tg3 driver in my batching
patches) pointed to me that skb->cb[] was being used to pass around vlan
data. 
This seems like a bad use since there can be a lot of things between 
a real hardware driver and something that sets a vlan tag (qdiscs come
to mind).

Creating a new skb field be the reasonable thing to do here but i know
that we are trying to avoid adding new fields. Thoughts?

cheers,
jamal 

Re: RFC: on [ab]use of skb->cb by VLAN code

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Jul 2007 20:59:16 -0400

> This seems like a bad use since there can be a lot of things between 
> a real hardware driver and something that sets a vlan tag (qdiscs come
> to mind).

I understand the concern, but how much qdisc stuff can possibly
happen between those two ->hard_start_xmit() calls and do we
want to support that in any way anyways?

The only alternative I see is to add more things to struct sk_buff
and that's usually very unpopular :-)

Re: RFC: on [ab]use of skb->cb by VLAN code

[jamal]

On Mon, 2007-30-07 at 18:33 -0700, David Miller wrote:

> I understand the concern, but how much qdisc stuff can possibly
> happen between those two ->hard_start_xmit() calls and do we
> want to support that in any way anyways?

>From a quick glance only netem seems to use it in the fast path (in a
legit way)
Theoretically, you could have many generations (i.e parents and
children, grandchildren etc) of netdevices stacked on top of each other
each with qdiscs. In a simple example: dont know how well these days
Vlans->bonding->somehardwarenetdevice works. 
Redirect will could also result in a graph of unrelated netdevices (and
it is fair game to trample on cb anywhere along the path)
I came across the issue because i used cb in batching to store transient
state which is used between qdisc dequeueing and hardware enqueueing
(looked and smelled legit to me).
 
> The only alternative I see is to add more things to struct sk_buff
> and that's usually very unpopular :-)

I know ;-> Thats why i asked the question.

cheers,
jamal

Re: RFC: on [ab]use of skb->cb by VLAN code

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Jul 2007 22:02:04 -0400

> I came across the issue because i used cb in batching to store transient
> state which is used between qdisc dequeueing and hardware enqueueing
> (looked and smelled legit to me).

Right, dequeue->device should be OK and doesn't work because of
the VLAN issue.

Re: RFC: on [ab]use of skb->cb by VLAN code

[Ben Greear]

David Miller wrote:
> From: jamal <hadi@cyberus.ca>
> Date: Mon, 30 Jul 2007 22:02:04 -0400
>
>   
>> I came across the issue because i used cb in batching to store transient
>> state which is used between qdisc dequeueing and hardware enqueueing
>> (looked and smelled legit to me).
>>     
>
> Right, dequeue->device should be OK and doesn't work because of
> the VLAN issue.
>   

So, shall we add a new field to the skb in order to get the info out of cb?

Looks like a single 32-bit field would be sufficient.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: RFC: on [ab]use of skb->cb by VLAN code

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Mon, 30 Jul 2007 21:30:19 -0700

> So, shall we add a new field to the skb in order to get the info out of cb?
> 
> Looks like a single 32-bit field would be sufficient.

I'm trying to brainstorm how to avoid consuming new space in
sk_buff and I'd like others to do so as well.

This isn't super urgent at all so I'll just think about it
on the backburner, things like the NAPI changes and dealing
with all of today's networking bug fixes is my top priority
at the moment.

Re: RFC: on [ab]use of skb->cb by VLAN code

[Ben Greear]

David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Mon, 30 Jul 2007 21:30:19 -0700
>
>   
>> So, shall we add a new field to the skb in order to get the info out of cb?
>>
>> Looks like a single 32-bit field would be sufficient.
>>     
>
> I'm trying to brainstorm how to avoid consuming new space in
> sk_buff and I'd like others to do so as well.
>
> This isn't super urgent at all so I'll just think about it
> on the backburner, things like the NAPI changes and dealing
> with all of today's networking bug fixes is my top priority
> at the moment.
>   
Ok.  From a quick look through skbuff.h, here is an idea:

Do we really need an 'unsigned int' for mac_len?  Maybe we could use
a 16-bit counter here, and then use the other 16 bits for the VLAN bits?
(I now think that only 16 bits are needed for VLAN, because it looks 
like the
NICs must know how to grab the vlan encapsulated protocol out of the 
skb??  Or,
maybe I just got confused when reading the vlan hwaccel logic...)

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: RFC: on [ab]use of skb->cb by VLAN code

[Rick Jones]

> Do we really need an 'unsigned int' for mac_len?  Maybe we could use
> a 16-bit counter here, and then use the other 16 bits for the VLAN bits?

Not knowing exactly if/how it interacts with that specific field I will 
point-out that IPoIB in OFED 1.2 just took their MTU to 65520.  While that 
doesn't break the bitbank it does get rather close.

rick jones

Re: RFC: on [ab]use of skb->cb by VLAN code

[Roland Dreier]

 > > Do we really need an 'unsigned int' for mac_len?  Maybe we could use
 > > a 16-bit counter here, and then use the other 16 bits for the VLAN bits?
 > 
 > Not knowing exactly if/how it interacts with that specific field I
 > will point-out that IPoIB in OFED 1.2 just took their MTU to 65520.
 > While that doesn't break the bitbank it does get rather close.

Leaving aside OFED releases, the IPoIB connected mode code in the
standard kernel also allows the MTU to go up to 65520.  And there's
nothing magic about that value -- we could easily do bigger packets.

However, this is irrelevant for two reasons: mac_len is the length of
the LL header, not the packet overall, *and* mac_len is already 16
bits as of commit 334a8132.

 - R.