[ovs-dev] [RFC v4 1/1] datapath: Add a new action check_pkt_len

Numan Siddique nusiddiq at redhat.com
Mon Mar 25 12:30:33 UTC 2019


On Mon, Mar 25, 2019 at 12:07 AM Numan Siddique <nusiddiq at redhat.com> wrote:

>
>
> On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192 at gmail.com> wrote:
>
>>
>> On 3/22/2019 2:58 AM, Numan Siddique wrote:
>>
>>
>>
>> On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192 at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On 3/21/2019 5:38 PM, Gregory Rose wrote:
>>> >
>>> >
>>> > On 3/21/2019 10:37 AM, Numan Siddique wrote:
>>> >> This is the datapath patch -
>>> https://patchwork.ozlabs.org/patch/1046370/
>>> >> and this is the corresponding ovs-vswitchd patch -
>>> >> https://patchwork.ozlabs.org/patch/1059081/ (this is part of the
>>> >> series -
>>> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=98190,
>>> >> but probably you would be interested in only ovs patch)
>>> >>
>>> >> Sharing the links so that you can find it easily.
>>> >>
>>> >> Thanks
>>> >> Numan
>>> >>
>>> >>
>>> >
>>> > This patch:
>>> >
>>> > https://patchwork.ozlabs.org/patch/1059081/
>>> >
>>> > shows this when applied:
>>> >
>>> > Applying: Add a new OVS action check_pkt_larger
>>> > .git/rebase-apply/patch:1097: new blank line at EOF.
>>> > +
>>> > warning: 1 line adds whitespace errors.
>>> >
>>> > In regards to the datapath patch 1046370
>>> > <https://patchwork.ozlabs.org/patch/1046370/>
>>> >
>>> > In execute_check_pkt_len():
>>> >
>>> > +
>>> > +       actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ?
>>> > VLAN_HLEN : 0);
>>> > +
>>> >
>>> > This doesn't seem right to me - the skb length should include the
>>> > length of the entire packet, including any
>>> > VLAN tags, or at least that is my understanding.  Please check it.
>>>
>>
> Hi Greg,
>
> I checked and tested it it. I can confirm that skb->len doesn't include
> the vlan header.
> Existing code in flow.c also uses the same way to get the packet len -
> https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77
>
>
I submitted the patch to net-dev ML and I forgot to CC - ovs-dev -
https://patchwork.ozlabs.org/patch/1063378/

Thanks
Numan


> Thanks
> Numan
>
> >
>>> > In validate_and_copy_check_pkt_len() in flow_netlink.c:
>>> >
>>> > +       static const struct nla_policy pol[OVS_CHECK_PKT_LEN_ATTR_MAX
>>> > + 1] = {
>>> > +               [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NLA_U16 },
>>> > +               [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = {
>>> > +                       .type = NLA_NESTED },
>>> > +               [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = {
>>> > +                       .type = NLA_NESTED },
>>> > +       };
>>> >
>>> > I don't care for declaring these things within function scope and it
>>> > is not generally done.  I see that
>>> > flow_netlink.c has one other instance of the nla_policy structure
>>> > statically declared within the function scope
>>> > but if you look at datapath.c none of them are.  I prefer the way it's
>>> > done in datapath.c.  I also grepped around
>>> > in other kernel code in the ./net tree and that is also the way it's
>>> > done there, i.e. I didn't see any other
>>> > instances of it declared within function scope.
>>> >
>>> > I compiled both the ovs-vswitchd and openvswitch kernel module
>>> > components with no issues.  I wanted to use
>>> > clang but the version of clang on Ubuntu right now doesn't have
>>> > retpoline support so it won't compile
>>> > kernel modules.
>>> >
>>> > :-/
>>> >
>>> > I did some quick regression testing and found no problems.  If you can
>>> > address the two coding issues I brought up
>>> > then I'd be glad to add my reviewed and tested by tags.
>>>
>>> Oh wait, this is just an RFC.
>>>
>>> I'll review and test the patches again when they officially come out.
>>> Maybe clang will have retpoline support
>>> by then.
>>>
>>>
>> Thank you for the review. I will address them. I am planning to submit
>> the patch
>> to net-next ML. Looks like the window is open now -
>> http://vger.kernel.org/~davem/net-next.html
>>
>> Please let me know in case you prefer to submit another RFC version here
>> before submitting to  net-dev ML.
>>
>>
>> I think you're good to go.
>>
>> Thanks,
>>
>> - Greg
>>
>>
>> Thanks
>> Numan
>>
>> - Greg
>>>
>>>
>>


More information about the dev mailing list