RFC: on [ab]use of skb->cb by VLAN code
Subsystems (2)
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
David Miller
maintainerFrom: jamal <[email protected]>
Date: Mon, 30 Jul 2007 20:59:16 -0400
Quoted 3 lines
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 :-)
Quoted 5 lines · David Miller
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).
Quoted 2 lines
The only alternative I see is to add more things to struct
sk_buffand that's usually very unpopular :-)
I know ;-> Thats why i asked the question.
cheers,
jamal
David Miller
maintainerFrom: jamal <[email protected]>
Date: Mon, 30 Jul 2007 22:02:04 -0400
Quoted 3 lines
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.
Quoted 4 lines · David Miller
From: jamal <[email protected]>
Date: Mon, 30 Jul 2007 22:02:04 -0400
Quoted 8 lines
Quoted 4 lines
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->deviceshould 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
Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
David Miller
maintainerFrom: Ben Greear <[email protected]>
Date: Mon, 30 Jul 2007 21:30:19 -0700
Quoted 3 lines
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.
Quoted 9 lines · David Miller
From: Ben Greear <[email protected]>
Date: Mon, 30 Jul 2007 21:30:19 -0700Quoted 4 lines
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.
Quoted 8 lines
I'm trying to brainstorm how to avoid consuming new space in
sk_buffand 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
Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 2 lines
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
Quoted 6 lines
Quoted 2 lines
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.