[ovs-dev] [PATCH 1/5] datapath: Use strip_vlan() for modify_vlan_tci().

[Jesse Gross]

On Thu, Jun 16, 2011 at 1:05 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Jun 16, 2011 at 12:51:21PM -0700, Jesse Gross wrote:
>> On Thu, Jun 16, 2011 at 12:37 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > On Wed, Jun 15, 2011 at 11:00:05AM -0700, Jesse Gross wrote:
>> >> The sematics for setting a vlan tag are to modify the existing tag
>> >> if one exists. ??This can be expressed as removing the existing tag
>> >> first and then adding a new one. ??This simplifies the code by not
>> >> requiring two copies of the logic that manipulates non-accelerated
>> >> vlans and should not make a performance difference because the vlan
>> >> tag is contained in a single cache line.
>> >>
>> >> Signed-off-by: Jesse Gross <jesse at nicira.com>
>> >
>> > Acked-by: Ben Pfaff <blp at nicira.com>
>> >
>> > But the test for VLAN_ETH_HLEN isn't needed, I think, because
>> > strip_vlan() does the same test.
>>
>> The issue is that if the length is not sufficient for a vlan tag then
>> strip_vlan() returns the skb, which is the same as what it does on
>> success and we will proceed to add a new vlan.  By checking first we
>> won't add a new vlan header and will immediately return.  This is what
>> we do for other actions and is consistent with the previous behavior
>> for modify_vlan_tci().
>
> Fair enough.
>
> I also think it would acceptable to just add the VLAN in that case.
> The packet was nonsensical before; adding a VLAN won't make it more or
> less nonsensical.

I guess although this seemed better as it is more consistent.

By the way, my eventual plan is to convert the kernel actions to be
pure push/pop for vlan tags as I think that really is the correct
model and make userspace generate a pop and push pair to emulate the
OpenFlow set action if necessary.

>> > strip_vlan() checks vlan_eth_hdr(skb)->h_vlan_proto for 802.1Q
>> > whereas modify_vlan_tci() checks skb->protocol. ??I don't know if
>> > consistency is important.
>>
>> I think that skb->protocol is the more canonical way to do it but I
>> can't think of a situation in which vlan_eth_hdr(skb)->h_vlan_proto
>> would be different, so I didn't bother to change it.
>
> OK.

Actually, I decided to just convert it to skb->protocol.