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

[Gregory Rose]

On 3/25/2019 5:30 AM, Numan Siddique wrote:
>
>
> On Mon, Mar 25, 2019 at 12:07 AM Numan Siddique <nusiddiq at redhat.com 
> <mailto:nusiddiq at redhat.com>> wrote:
>
>
>
>     On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192 at gmail.com
>     <mailto: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 <mailto: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/

Yes, I'm subscribed to netdev as well and already saw it.  :)

Thanks!

- Greg

>
> 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
>>
>