On Tue, Oct 27, 2015 at 9:45 AM, Thomas F Herbert
<thomasfherb...@gmail.com> wrote:
> On 10/26/15 10:10 PM, Pravin Shelar wrote:
> Thanks for the review.
>>
>> On Sun, Oct 25, 2015 at 5:11 PM, Thomas F Herbert
>> <thomasfherb...@gmail.com> wrote:
>>>
>>> Add support for 802.1ad including the ability to push and pop double
>>> tagged vlans. Add support for 802.1ad to netlink parsing and flow
>>> conversion. Uses double nested encap attributes to represent double
>>> tagged vlan. Inner TPID encoded along with ctci in nested attributes.
>>> Outer
>>> TPID is also encoded in the flow key.
>>>
>>> Signed-off-by: Thomas F Herbert <thomasfherb...@gmail.com>
>>
>> This patch does not apply on current master due to conflicts related
>> net-branch merge.
>
> OK, I will rebase.
>
>>
>>> ---
>>> net/openvswitch/actions.c | 6 +-
>>> net/openvswitch/flow.c | 76 ++++++++++++----
>>> net/openvswitch/flow.h | 8 +-
>>> net/openvswitch/flow_netlink.c | 199
>>> +++++++++++++++++++++++++++++++++++++----
>>> net/openvswitch/vport-netdev.c | 4 +-
>>> 5 files changed, 252 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>>> index c8db44a..ed19e2b 100644
>>> --- a/net/openvswitch/flow.c
>>> +++ b/net/openvswitch/flow.c
>>> @@ -302,24 +302,68 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>> sizeof(struct icmp6hdr));
>>> }
>>>
>>> -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> +/* Parse vlan tag from vlan header.
>>> + * Returns ERROR on memory error.
>>> + * Returns 0 if it encounters a non-vlan or incomplete packet.
>>> + * Returns 1 after successfully parsing vlan tag.
>>> + */
>>> +
>>> +static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *vlan)
>>> {
>>> - struct qtag_prefix {
>>> - __be16 eth_type; /* ETH_P_8021Q */
>>> - __be16 tci;
>>> - };
>>> - struct qtag_prefix *qp;
>>> + struct vlan_head *qp = (struct vlan_head *)skb->data;
>>> +
>>> + if (likely(!eth_type_vlan(qp->tpid)))
>>> + return 0;
>>>
>>> - if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> sizeof(__be16)))
>>> + if (unlikely(skb->len < sizeof(struct vlan_head) +
>>> sizeof(__be16)))
>>> return 0;
>>
>> Why do we need extra sizeof(__be16) bytes here?
>
> I don't have an answer to your question. I didn't write this code and have
> wondered about why the extra two bytes were reserved. I don't know why it
> should be necessarily for inner or outer vlans or the HW accelerated case or
> for the non-accelerated case. If no reviewer can state a case for it, I will
> remove it with the next version of this patch.
>
Looks like it is optimization for parsing ethertype, So lets keep it.
>>>
>>> } else if (!tci) {
>>> /* Corner case for truncated 802.1Q header. */
>>> if (nla_len(encap)) {
>>> @@ -1169,7 +1312,7 @@ int ovs_nla_get_match(struct net *net, struct
>>> sw_flow_match *match,
>>> goto free_newmask;
>>>
>>> /* Always match on tci. */
>>> - SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>> + SW_FLOW_KEY_PUT(match, eth.vlan.tci, htons(0xffff),
>>> true);
>>
>> Also need to exact match on inner tci.
>
> This code sets a match on tci even if no vlan is present. Is this is for the
> case where there is no explicit mask specified in the netlink encoded flow?
> If that is correct, then it does need to be done for the inner vlan too.
Yes, By default it needs to be matched. userspace can overwrite it
with different wildcard.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
