[ovs-dev] [PATCH net-next V16 3/3] openvswitch: 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
Thomas F Herbert
thomasfherbert at gmail.com
Fri Oct 16 02:03:25 UTC 2015
On 10/15/15 8:37 PM, Pravin Shelar wrote:
> On Thu, Oct 15, 2015 at 4:48 PM, Thomas F Herbert
> <thomasfherbert at gmail.com> wrote:
>> On 10/15/15 7:02 PM, Pravin Shelar wrote:
>> Thanks for the review. See my comment below.
>>
>> --TFH
>>
>>
>>> On Thu, Oct 15, 2015 at 7:01 AM, Thomas F Herbert
>>> <thomasfherbert at 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.
>>>>
>>>> Signed-off-by: Thomas F Herbert <thomasfherbert at gmail.com>
>>>> ---
>>>> net/openvswitch/actions.c | 6 +-
>>>> net/openvswitch/flow.c | 75 ++++++++++++++----
>>>> net/openvswitch/flow.h | 8 +-
>>>> net/openvswitch/flow_netlink.c | 169
>>>> +++++++++++++++++++++++++++++++++++++----
>>>> net/openvswitch/vport-netdev.c | 4 +-
>>>> 5 files changed, 228 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>> index 315f533..09cc1c9 100644
>>>> --- a/net/openvswitch/actions.c
>>>> +++ b/net/openvswitch/actions.c
>>>> @@ -236,7 +236,8 @@ static int pop_vlan(struct sk_buff *skb, struct
>>>> sw_flow_key *key)
>>>> if (skb_vlan_tag_present(skb))
>>>> invalidate_flow_key(key);
>>>> else
>>>> - key->eth.tci = 0;
>>>> + key->eth.vlan.tci = 0;
>>>> + key->eth.vlan.tpid = 0;
>>>> return err;
>>>> }
>>>>
>>>> @@ -246,7 +247,8 @@ static int push_vlan(struct sk_buff *skb, struct
>>>> sw_flow_key *key,
>>>> if (skb_vlan_tag_present(skb))
>>>> invalidate_flow_key(key);
>>>> else
>>>> - key->eth.tci = vlan->vlan_tci;
>>>> + key->eth.vlan.tci = vlan->vlan_tci;
>>>> + key->eth.vlan.tpid = vlan->vlan_tpid;
>>>> return skb_vlan_push(skb, vlan->vlan_tpid,
>>>> ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>>>> }
>>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>>>> index c8db44a..8a4e298 100644
>>>> --- a/net/openvswitch/flow.c
>>>> +++ b/net/openvswitch/flow.c
>>>> @@ -302,24 +302,69 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>>> sizeof(struct icmp6hdr));
>>>> }
>>>>
>>>> -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>>> +struct qtag_prefix {
>>>> + __be16 eth_type; /* ETH_P_8021Q or ETH_P_8021AD */
>>>> + __be16 tci;
>>>> +};
>>>> +
>>> Now we can just use newly defined struct vlan_header here.
>>>
>>>> +/* 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, __be16 vlan_proto,
>>>> + __be16 vlan_tci, struct vlan_head *vlan)
>>>> {
>>>> - struct qtag_prefix {
>>>> - __be16 eth_type; /* ETH_P_8021Q */
>>>> - __be16 tci;
>>>> - };
>>>> - struct qtag_prefix *qp;
>>>> + if (likely(!eth_type_vlan(vlan_proto)))
>>>> + return 0;
>>>>
>>>> if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>>> sizeof(__be16)))
>>>> return 0;
>>>>
>>>> if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>>> - sizeof(__be16))))
>>>> + sizeof(__be16))))
>>>> return -ENOMEM;
>>>>
>>>> - qp = (struct qtag_prefix *) skb->data;
>>>> - key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>>> + vlan->tci = vlan_tci | htons(VLAN_TAG_PRESENT);
>>>> + vlan->tpid = vlan_proto;
>>>> +
>>>> __skb_pull(skb, sizeof(struct qtag_prefix));
>>>> + return 1;
>>>> +}
>>>> +
>>>> +static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>>> +{
>>>> + struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
>>>> + int res;
>>>> +
>>>> + if (likely(skb_vlan_tag_present(skb))) {
>>>> + key->eth.vlan.tci = htons(skb->vlan_tci);
>>>> + key->eth.vlan.tpid = skb->vlan_proto;
>>>> +
>>>> + /* Case where ingress processing has already stripped
>>>> + * the outer vlan tag.
>>>> + */
>>>> + res = parse_vlan_tag(skb, qp->eth_type, qp->tci,
>>>> + &key->eth.cvlan);
>>>> + if (res < 0)
>>>> + return res;
>>>> + /* For inner tag, return 0 because neither
>>>> + * non-existant nor partial inner tag is an error.
>>>> + */
>>>> + return 0;
>>>> + }
>>>> + res = parse_vlan_tag(skb, qp->eth_type, qp->tci, &key->eth.vlan);
>>>> + if (res <= 0)
>>>> + /* This is an outer tag in the non-accelerated VLAN
>>>> + * case. Return error unless it is a complete vlan tag.
>>>> + */
>>>> + return res;
>>>> +
>>>> + /* Parse inner vlan tag if present for non-accelerated case. */
>>>> + res = parse_vlan_tag(skb, qp->eth_type, qp->tci,
>>>> &key->eth.cvlan);
>>>> + if (res <= 0)
>>>> + return res;
>>>>
>>> same qp pointer is passed for inner and outer vlan parameters here. It
>>> is better to just pass skb and keep qp inside parse_vlan_tag()
>>> function.
>>>
>>>> return 0;
>>>> }
>>>> @@ -480,12 +525,12 @@ static int key_extract(struct sk_buff *skb, struct
>>>> sw_flow_key *key)
>>>> * update skb->csum here.
>>>> */
>>>>
>>>> - key->eth.tci = 0;
>>>> - if (skb_vlan_tag_present(skb))
>>>> - key->eth.tci = htons(skb->vlan_tci);
>>>> - else if (eth->h_proto == htons(ETH_P_8021Q))
>>>> - if (unlikely(parse_vlan(skb, key)))
>>>> - return -ENOMEM;
>>>> + key->eth.vlan.tci = 0;
>>>> + key->eth.vlan.tpid = 0;
>>>> + key->eth.cvlan.tci = 0;
>>>> + key->eth.cvlan.tpid = 0;
>>> Lets move this over to parse_vlan().
>>>
>>>> + if (unlikely(parse_vlan(skb, key)))
>>>> + return -ENOMEM;
>>>>
>>>> key->eth.type = parse_ethertype(skb);
>>>> if (unlikely(key->eth.type == htons(0)))
>>>> diff --git a/net/openvswitch/flow_netlink.c
>>>> b/net/openvswitch/flow_netlink.c
>>>> index c92d6a2..5cff83c 100644
>>>> --- a/net/openvswitch/flow_netlink.c
>>>> +++ b/net/openvswitch/flow_netlink.c
>>> ...
>>>
>>>
>>>> static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match
>>>> *match,
>>>> u64 attrs, const struct nlattr **a,
>>>> bool is_mask, bool log)
>>>> @@ -845,7 +875,7 @@ static int ovs_key_from_nlattrs(struct net *net,
>>>> struct sw_flow_match *match,
>>>> return -EINVAL;
>>>> }
>>>>
>>>> - SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
>>>> + SW_FLOW_KEY_PUT(match, eth.vlan.tci, tci, is_mask);
>>>> attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
>>>> }
>>>>
>>>> @@ -1064,6 +1094,86 @@ static void mask_set_nlattr(struct nlattr *attr,
>>>> u8 val)
>>>> nlattr_set(attr, val, ovs_key_lens);
>>>> }
>>>>
>>>> +static int parse_vlan_from_nlattrs(const struct nlattr **nla,
>>>> + struct sw_flow_match *match,
>>>> + u64 *key_attrs, bool *ie_valid,
>>>> + const struct nlattr **a, bool is_mask,
>>>> + bool log)
>>>> +{
>>>> + int err;
>>>> + const struct nlattr *encap;
>>>> +
>>>> + if (!is_mask) {
>>>> + u64 v_attrs = 0;
>>>> +
>>> attributes does not need 64 bits, 32 bits are sufficient.
>> Yes, there certainly not more then 32 attributes in this layer of nesting
>> but in the parse_flow_nlattrs() function argument 3 is a u64 *
>> Don't you think this might be dangerous? Maybe are you saying that I should
>> rewrite that generic function to only support a maximum of 32 netlink
>> attributes per level of nesting. The current OVS kernel module is optimized
>> for 64 bit architectures where there is no extra cost for a 64 bit value and
>> I think what you are suggesting might go beyond the scope of this patch. If
>> it is a good idea, shouldn't it be considered for a separate patch?
>>
> OK. Lets keep it as it is.
>
>> --TFH
>>>> + err = parse_flow_nlattrs(*nla, a, &v_attrs, log);
>>>> + if (err)
>>>> + return err;
>>>> + /* Another encap attribute here indicates
>>>> + * the presence of a double tagged vlan.
>>>> + */
>>>> + if ((v_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
>>>> +
>>>> eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
>>>> + if (!((v_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) &&
>>>> + (v_attrs & (1ULL << OVS_KEY_ATTR_ENCAP))))
>>>> {
>>> After changing v_attrs type, there is no need to use 1ULL.
>> See remark above.
>>
> Just to be consistent, lets use 32 bit value of one here and other
> such instances.
OK, I agree. The 1ULL is not necessary in my opinion.
More information about the dev
mailing list