From: Thomas F Herbert <therbert@redhat•com>
To: Pravin Shelar <pshelar@nicira•com>,
Thomas F Herbert <thomasfherbert@gmail•com>
Cc: netdev <netdev@vger•kernel.org>,
"dev@openvswitch•org" <dev@openvswitch•org>
Subject: Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
Date: Thu, 29 Oct 2015 19:47:15 -0400 [thread overview]
Message-ID: <5632B003.7050206@redhat.com> (raw)
In-Reply-To: <CALnjE+pBSdjVt0AvyK9ziL_tDEzaxEDEbW_Eqk=cKABi9vFCAQ@mail.gmail.com>
Pravin, please look at comment inline below:
On 10/27/15 1:22 PM, Pravin Shelar wrote:
> On Tue, Oct 27, 2015 at 9:45 AM, Thomas F Herbert
> <thomasfherbert@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
>>> <thomasfherbert@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 <thomasfherbert@gmail•com>
>>> This patch does not apply on current master due to conflicts related
>>> net-branch merge.
>> OK, I will rebase.
Pravin, I implemented all reviewer comments and completing rebasing to
latest net-next upstream. The patch works well and flows install
properly. However, there may be one remaining issue. The vport-dev
transmit function now calls dev_queue_transmit() directly because it is
registered as the vport send op. The affect of this is that the vlan mtu
adjustment code in ovs_netdev_send() that was patched for mtu adjustment
for single and double tagged vlans is gone.Could you please verify that
the size adjustment is no longer needed.
>>
>>>> ---
>>>> 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.
--
Thomas F Herbert Red Hat
prev parent reply other threads:[~2015-10-29 23:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-26 0:11 [PATCH net-next V18 0/3] openvswitch: Add support for 802.1ad Thomas F Herbert
[not found] ` <1445818286-4870-1-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-26 0:11 ` [PATCH net-next V17 1/3] openvswitch: 802.1ad uapi changes Thomas F Herbert
2015-10-26 0:11 ` [PATCH net-next V17 2/3] Check for vlan ethernet types for 8021.q or 802.1ad Thomas F Herbert
2015-10-26 12:14 ` Albino B Neto
2015-10-27 12:04 ` Thomas F Herbert
2015-10-26 0:11 ` [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes Thomas F Herbert
[not found] ` <1445818286-4870-4-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-27 2:10 ` Pravin Shelar
[not found] ` <CALnjE+rcP1trUt2zSSGoUF9r1XwNgtRiLgsS2soejhd4j0zyHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-10-27 16:45 ` Thomas F Herbert
[not found] ` <562FAA0E.4090908-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-27 17:22 ` Pravin Shelar
2015-10-29 23:47 ` Thomas F Herbert [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5632B003.7050206@redhat.com \
--to=therbert@redhat$(echo .)com \
--cc=dev@openvswitch$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pshelar@nicira$(echo .)com \
--cc=thomasfherbert@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox