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

Numan Siddique nusiddiq at redhat.com
Tue Feb 12 12:12:22 UTC 2019


Thanks Ben and Lorenzo for the review and comments.

Few comments inline.


On Tue, Feb 12, 2019 at 7:57 AM Ben Pfaff <blp at ovn.org> wrote:

> Greg, I recommend that you take a look at this one.
>
> (More commentary below.)
>
> On Thu, Jan 10, 2019 at 11:29:48PM +0530, nusiddiq at redhat.com wrote:
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > [Please note, this patch is submitted as RFC in ovs-dev ML to
> > get feedback before submitting to netdev ML]
> >
> > This patch adds a new action which checks the packet length and
> > executes a set of actions if the packet length is greater than
> > the specified length or executes another set of actions if the
> > packet length is lesser or equal to.
> >
> > This action takes below nlattrs
> >   * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
> >     to apply if the packet length is greater than the specified 'pkt_len'
> >
> >   * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
> >     actions to apply if the packet length is lesser or equal to the
> >     specified 'pkt_len'.
> >
> > The main use case for adding this action is to solve the packet
> > drops because of MTU mismatch in OVN virtual networking solution.
> > When a VM (which belongs to a logical switch of OVN) sends a packet
> > destined to go via the gateway router and if the nic which provides
> > external connectivity, has a lesser MTU, OVS drops the packet
> > if the packet length is greater than this MTU.
> >
> > With the help of this action, OVN will check the packet length
> > and if it is greater than the MTU size, it will generate an
> > ICMP packet (type 3, code 4) and includes the next hop mtu in it
> > so that the sender can fragment the packets.
>
> I'm not used to seeing series where different patches apply to different
> repos!  It took me a while to figure out why "git am" didn't want to
> work at all (but then I actually looked at the patch).
>

Thanks for looking at the patch. The main goal of including this datapath
patch
in this series is to get initial feedback about the overall approach and get
comments from the ovs datapath developers before submitting the patch to
net-next ML.
I mentioned this in RFC v3 0/5 patch. I think patchwork doesn't show  the
patch 0.



> I didn't have a net-next tree handy so my comments are based on reading
> the patch without applying or building it.
>
> There is a special issue with this action, which is a lot like a special
> issue with the "sample".  That is that the action has flow control that
> might change the flow in different ways, and this can make an important
> difference for actions that follow this one.  If one fork of the action
> pops off a header, and the other one does not, then that makes
> validating actions that come after it complicated.  I do not see
> anything here that takes that into account.  I recommend reading the
> validation code for the sample action for inspiration.
>
>
Sure. I will look into it.


> In validate_and_copy_check_pkt_len(), it might be better to use
> nla_policy and nla_parse_nested().  Or maybe not.  I did not look
> closely.
>

Ack.


>
> I don't think that execute_check_pkt_len() should need to check for
> netlink format errors.  The validation code should take care of that.
> It might also be able to assume a particular order for the attributes,
> depending on how you implement validation.
>

Ack.


>
> I'm still not thrilled by the naming.  I don't have any wonderful names
> though.
>

I am sorry. I couldn't think of any other name.

Does the action - "cpl_execute" or "cpl_exec" sounds a little bit better ?
cpl = check pkt len.

Thanks again for looking into the patch series providing the comments.

I will wait for Greg's comments. And then if it's fine, I will address the
commetns
and  will submit the patch to the net-next ML as a formal patch.

Thanks
Numan


More information about the dev mailing list