[ovs-dev] [PATCH 1/2 V5] Linux datapath changes to support 802.1AD

[Thomas F Herbert]

Flavio,

Thanks for your review. I am preparing a version 6 of the patch in which 
I will be fixing the issues you pointed out but I am interested in 
further guidance in your responses below.

Thanks, Tom


On 12/30/14, 3:30 PM, Flavio Leitner wrote:
> On Tuesday, December 30, 2014 10:12:59 AM you wrote:
>> This is the linux kernel datapath portion of the 802.1AD patch.
>>
>>
>> Signed-off-by: Thomas F Herbert <thomasfherbert at entpnt.com>
>>
>> ---
>>   datapath/actions.c                                | 32 ++++++---
>>   datapath/flow.c                                   | 80 +++++++++++++++++++----
>>   datapath/flow.h                                   |  1 +
>>   datapath/flow_netlink.c                           | 62 ++++++++++++++++--
>>   datapath/linux/compat/include/linux/openvswitch.h | 16 ++---
>>   5 files changed, 153 insertions(+), 38 deletions(-)
>>
...
>>
>>   
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index 69b13b3..d549fae 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
...
>> +
>> +			if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +					sizeof(__be16)))
>> +				return 0;
> This check is included in the pskb_may_pull() below.
This puzzles me too. The double headroom check looks redundant but it is 
leveraged from the existing  code that does it for 802.1q single vlans. 
Please advise, should we change this everywhere?
>
>> +
>> +			if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> +					sizeof(__be16)))) {
>> +				return -ENOMEM;
>> +			}
>> +
This is also leveraged from existing vlan code that checks for headroom 
for the vlan header plus the encapsulated ethertype. I didn't change 
this  except extending it for a full 802.1ad vlan header. Please advise, 
should we change this everywhere?
> I'd suggest to use offsetof(struct  qtag_prefix, ctci), but I am not really
> sure if it's portable. Or simply define the ctci_offset above as
> sizeof(struct qtag_prefix) + sizeof(__be16) for better readability.
>
>> +			if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
>> +				key->eth.ctci = qp->tci | htons(VLAN_TAG_PRESENT);
>> +				__skb_pull(skb, sizeof(struct qtag_prefix));
>> +			}
>> +		}
>> +                return 0;
>> +	}
....
> Thanks
> fbl


-- 
Thomas F. Herbert