[ovs-dev] [PATCH v3 1/6] Add a new OVS action check_pkt_larger

Numan Siddique nusiddiq at redhat.com
Fri Apr 19 17:36:02 UTC 2019


Thank you Ben and Greg for the reviews.

Please see few comments inline below

Thanks
Numan


On Fri, Apr 19, 2019 at 4:58 AM Gregory Rose <gvrose8192 at gmail.com> wrote:

>
> On 4/18/2019 2:48 PM, Ben Pfaff wrote:
> > On Tue, Apr 16, 2019 at 01:23:37PM +0530, nusiddiq at redhat.com wrote:
> >> From: Numan Siddique <nusiddiq at redhat.com>
> >>
> >> This patch adds a new action 'check_pkt_larger' which checks if the
> >> packet is larger than the given size and stores the result in the
> >> destination register.
> > Thanks for the revision!  I'm taking a detailed look at this series now
> > that the datapath action is upstream and reviewed for backport.
> >
> > (In my opinion, it would be better if the datapath backport was patch 1,
> > but it is not a big deal.)
>
>
If I had to make datapath backport as patch 1, then to address the compiler
warnings,
I had to add the "case OVS_ACTION_ATTR_CHECK_PKT_LEN" in some of the switch
statements in the files like lib/dpif.c, lib/dpif-netdev.c etc.

That's why I ordered this way and hope it's fine :).


FWIW patches 1 and 2 of the series apply well independently or
> together.  All of my testing was done
> with both patches 1 and 2 applied.  However, I did not do a full review
> of all the code in the first patch of the
> series due to time constraints.  Patch 1 of the series did apply cleanly
> and passed checkpatch.
>
> - Greg
>
> >
> > odp_execute_check_pkt_len() checks the action pretty carefully.  It
> > might not be necessary, since the action should be generated by code
> > elsewhere in the same process.  If you decide that you like the checks
> > anyway (I do not object), then:
> >
> >      * NL_NESTED_FOR_EACH is safer than NL_NESTED_FOR_EACH_UNSAFE, so you
> >        might consider it.
> >
> >      * ovs_assert() is more usual than if (...) { OVS_NOT_REACHED(); }
> >
> > odp_execute_check_pkt_len() could use nl_parse_nested(), like
> > format_odp_check_pkt_len_action() does.  nl_parse_nested() does thorough
> > error checking internally.  The only reason the odp-execute.c functions
> > tend not to use nl_parse_nested() and friends is because they trust the
> > action.  If you prefer not to trust it, nl_parse_nested() might be
> > simpler.
>

Ack. Will do


> >
> > There's a missing blank line here:
>

Ack. Will do.


> >
> > @@ -353,10 +353,12 @@ enum ofp_raw_action_type {
> >       NXAST_RAW_DECAP,
> >
> >       /* NX1.3+(48): void. */
> >       NXAST_RAW_DEC_NSH_TTL,
> >
> > +    /* NX1.0+(49): struct nx_action_check_pkt_larger, ... VLMFF */
> > +    NXAST_RAW_CHECK_PKT_LARGER,
> >   /* ## ------------------ ## */
> >   /* ## Debugging actions. ## */
> >   /* ## ------------------ ## */
> >
> >   /* These are intentionally undocumented, subject to change, and
> ovs-vswitchd */
> >
> > struct nx_action_check_pkt_larger encodes a number of bits, but only a
> > single bit is allowed.  Thus, the action structure shouldn't have a
> > space for a count of bits.
> >
>

Ack. Will do.


> > Somewhere, we need to document how the packet's size is calculated.
> > Probably this should be in lib/ovs-actions.xml.  The example there
> > implies that the argument includes the L2 payload but not the L2 header,
>

Actually, the packet size specified in the action includes the length of the
entire packet which includes L2 header and L2 payload. But it doesn't
take VLAN header length into account.

Below is the comment I got from Pravin for the datapath patch

******
> +       cpl_arg = nla_data(attr);
> +       arg = nla_data(cpl_arg);
> +
> +       actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ?
VLAN_HLEN : 0);
> +
>From Pravin >>> I do not see need to add vlan-tag length here. This limit
can be
programmed in vlan flow itself. So that we can avoid this calculation
in fast path.

https://patchwork.ozlabs.org/patch/1063378/
***

So I changed the code to check the packet length this way in the datapath
patch
***
if (skb->len <= arg->pkt_len) {
...
..
***


I will document this the lib/ovs-actions.xml as you have suggested.

Thanks

> so that a maximum-length Ethernet packet is 1500 bytes (not 1514).  If
> > there are any other special cases or exceptions (for example, does a
> > VLAN header count against the size limit?), then it would be good to
> > describe them there too.
> >
> > Thanks,
> >
> > Ben.
>
>


More information about the dev mailing list