diff mbox series

[ovs-dev,RFC,v3,2/5] Add a new OVS action check_pkt_larger

Message ID 20190110180033.4734-1-nusiddiq@redhat.com
State RFC
Headers show
Series Address MTU issue for larger packets in OVN | expand

Commit Message

Numan Siddique Jan. 10, 2019, 6 p.m. UTC
From: Numan Siddique <nusiddiq@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.

Usage: check_pkt_larger:len->REGISTER
Eg. match=...,actions=check_pkt_larger:1442->NXM_NX_REG0[0],next;

This patch makes use of the new datapath action - 'check_pkt_len'
which is still WIP. At the start of ovs-vswitchd, datapath is probed
for this action. If the datapath action is present, then 'check_pkt_larger'
makes use of this datapath action.

Datapath action 'check_pkt_len' takes these 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'.

Let's say we have these flows added to an OVS bridge br-int

table=0, priority=100 in_port=1,ip,actions=check_pkt_larger:100->NXM_NX_REG0[0],resubmit(,1)
table=1, priority=200,in_port=1,ip,reg0=0x1/0x1 actions=output:3
table=1, priority=100,in_port=1,ip,actions=output:4

Suppose if a packet is received from in_port=1, the packet is sent
to userspace (MISS_UPCALL). The action 'check_pkt_larger' will be translated as
  - check_pkt_len( > 100 ? (3) : (4))

datapath will check the packet length and if the packet length is greater than 100,
it will be output to port 3, else it will be output to port 4.

In case, datapath doesn't support 'check_pkt_len' action, the OVS action
'check_pkt_larger' sets SLOW_ACTION so that datapath flow is not added.

This OVS action is intended to be used by OVN to check the packet length
and generate an ICMP packet with type 3, code 4 and next hop mtu
in the logical router pipeline if the MTU of the physical interface
is lesser than the packet length. More information can be found here [1]

TODO:
 - Add test cases.
 - Add documentation

Request to suggest a better name for the action in case 'check_pkt_larger'
seems odd.

[1] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
Suggested-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
CC: Ben Pfaff <blp@ovn.org>
---
 .../linux/compat/include/linux/openvswitch.h  |  30 +++--
 include/openvswitch/ofp-actions.h             |  18 +++
 lib/dpif-netdev.c                             |   1 +
 lib/dpif.c                                    |   1 +
 lib/odp-execute.c                             |  72 ++++++++++++
 lib/odp-util.c                                |  36 ++++++
 lib/ofp-actions.c                             |  98 +++++++++++++++-
 lib/ofp-parse.c                               |  10 ++
 ofproto/ofproto-dpif-ipfix.c                  |   1 +
 ofproto/ofproto-dpif-sflow.c                  |   1 +
 ofproto/ofproto-dpif-xlate.c                  | 109 ++++++++++++++++++
 ofproto/ofproto-dpif.c                        |  47 ++++++++
 ofproto/ofproto-dpif.h                        |   5 +-
 13 files changed, 418 insertions(+), 11 deletions(-)

Comments

Ben Pfaff Feb. 12, 2019, 3:02 a.m. UTC | #1
On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@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.

This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the
"userspace only" group of datapath actions.  It should not do that.
It should also not remove OVS_CLONE_ATTR_EXEC.

ofpact_remaining_len() should not return bool.  (This mistake suggests
that the case where it should return nonzero was not tested.)

I don't understand the check for check_pkt_larger->dst.field in
xlate_check_pkt_larger().  The action would be invalid if this was
missing.  Such an ofpact should not be translated.

The following comment appears incomplete:
        /* If datapath doesn't support check_pkt_len action, then set the
         * SLOW_ACTION flag so that ..
         */

odp-util.c has code for formatting the new action, but not for parsing.
We need both.

The format might be a little too cute.  It might be wise to use
something more like check_pkt_len(size=123, gt(...), le(...)).

decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field
specified and the bits in it are valid and usable and return an error if
they are not.

As is, the OpenFlow action format only allows NXM headers.  All new
actions should also support OXM 64-bit experimenter headers.  Look
around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header()
for more information.

The OpenFlow syntax looks really weird.  I don't understand it at all.

I doubt that the action translation handles the case where freezing has
to happen properly.  I don't think we have any other actions where
there are two opportunities to freeze translation.  This might require
novel work.

In check_check_pkt_len(), the inner ct_clear action kind of mixes up two
different checks.  I don't think that check_pkt_len should rely on
ct_clear.  I recommend putting some other action inside, if one is
needed.

There isn't any documentation, but some is needed.

Tests are needed.

A NEWS item is appropriate.

Thanks,

Ben.
Numan Siddique Feb. 13, 2019, 5:23 p.m. UTC | #2
Thanks Ben for providing the review comments.

I will address all the comments.

A couple of comments inline.

Thanks
Numan


On Tue, Feb 12, 2019 at 8:33 AM Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@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.
>
> This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the
> "userspace only" group of datapath actions.  It should not do that.
> It should also not remove OVS_CLONE_ATTR_EXEC.
>
> ofpact_remaining_len() should not return bool.  (This mistake suggests
> that the case where it should return nonzero was not tested.)
>
> I don't understand the check for check_pkt_larger->dst.field in
> xlate_check_pkt_larger().  The action would be invalid if this was
> missing.  Such an ofpact should not be translated.
>
> The following comment appears incomplete:
>         /* If datapath doesn't support check_pkt_len action, then set the
>          * SLOW_ACTION flag so that ..
>          */
>
> odp-util.c has code for formatting the new action, but not for parsing.
> We need both.
>
> The format might be a little too cute.  It might be wise to use
> something more like check_pkt_len(size=123, gt(...), le(...)).
>
> decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field
> specified and the bits in it are valid and usable and return an error if
> they are not.
>
> As is, the OpenFlow action format only allows NXM headers.  All new
> actions should also support OXM 64-bit experimenter headers.  Look
> around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header()
> for more information.
>
> The OpenFlow syntax looks really weird.  I don't understand it at all.
>

By OpenFlow syntax you mean the way this action is being used ?

i.e  "check_pkt_larger(1500)->NXM_NX_REG0[0]" ?

I  referred the existing action load and tried to use it the same way.
I will think of a better way.

Thanks


set

>
> I doubt that the action translation handles the case where freezing has
> to happen properly.  I don't think we have any other actions where
> there are two opportunities to freeze translation.  This might require
> novel work.
>
> In check_check_pkt_len(), the inner ct_clear action kind of mixes up two
> different checks.  I don't think that check_pkt_len should rely on
> ct_clear.  I recommend putting some other action inside, if one is
> needed.
>
> There isn't any documentation, but some is needed.
>
> Tests are needed.
>

Ack.
I didn't work on these as I wanted to get feedback about the approach in
general.

Thanks
Numan



> A NEWS item is appropriate.
>
> Thanks,
>
> Ben.
>
Ben Pfaff Feb. 13, 2019, 5:36 p.m. UTC | #3
On Wed, Feb 13, 2019 at 10:53:53PM +0530, Numan Siddique wrote:
> On Tue, Feb 12, 2019 at 8:33 AM Ben Pfaff <blp@ovn.org> wrote:
> 
> > The OpenFlow syntax looks really weird.  I don't understand it at all.
> >
> 
> By OpenFlow syntax you mean the way this action is being used ?
> 
> i.e  "check_pkt_larger(1500)->NXM_NX_REG0[0]" ?
> 
> I  referred the existing action load and tried to use it the same way.
> I will think of a better way.

That's what I meant.

(Like some of my other comments, this is a complaint where I don't have
a better alternative to propose.)

> > There isn't any documentation, but some is needed.
> >
> > Tests are needed.
> >
> 
> Ack.
> I didn't work on these as I wanted to get feedback about the approach in
> general.

Sure, understood.
Numan Siddique March 20, 2019, 11:44 a.m. UTC | #4
Hi Ben,

Few comments inline.

Thanks
Numan

On Wed, Feb 13, 2019 at 10:53 PM Numan Siddique <nusiddiq@redhat.com> wrote:

>
> Thanks Ben for providing the review comments.
>
> I will address all the comments.
>
> A couple of comments inline.
>
> Thanks
> Numan
>
>
> On Tue, Feb 12, 2019 at 8:33 AM Ben Pfaff <blp@ovn.org> wrote:
>
>> On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusiddiq@redhat.com wrote:
>> > From: Numan Siddique <nusiddiq@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.
>>
>> This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the
>> "userspace only" group of datapath actions.  It should not do that.
>> It should also not remove OVS_CLONE_ATTR_EXEC.
>>
>> ofpact_remaining_len() should not return bool.  (This mistake suggests
>> that the case where it should return nonzero was not tested.)
>>
>> I don't understand the check for check_pkt_larger->dst.field in
>> xlate_check_pkt_larger().  The action would be invalid if this was
>> missing.  Such an ofpact should not be translated.
>>
>> The following comment appears incomplete:
>>         /* If datapath doesn't support check_pkt_len action, then set the
>>          * SLOW_ACTION flag so that ..
>>          */
>>
>> odp-util.c has code for formatting the new action, but not for parsing.
>> We need both.
>>
>> The format might be a little too cute.  It might be wise to use
>> something more like check_pkt_len(size=123, gt(...), le(...)).
>>
>> decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field
>> specified and the bits in it are valid and usable and return an error if
>> they are not.
>>
>> As is, the OpenFlow action format only allows NXM headers.  All new
>> actions should also support OXM 64-bit experimenter headers.  Look
>> around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header()
>> for more information.
>>
>> The OpenFlow syntax looks really weird.  I don't understand it at all.
>>
>
> By OpenFlow syntax you mean the way this action is being used ?
>
> i.e  "check_pkt_larger(1500)->NXM_NX_REG0[0]" ?
>
> I  referred the existing action load and tried to use it the same way.
> I will think of a better way.
>
>
Unfortunately, in v4 it's still the same syntax. I will keep exploring.


> Thanks
>
>
> set
>
>>
>> I doubt that the action translation handles the case where freezing has
>> to happen properly.  I don't think we have any other actions where
>> there are two opportunities to freeze translation.  This might require
>> novel work.
>>
>>
In v4 of this patch, with little work, freezing seems to be happening
properly.
I have added the test case for this scenario as well.

In ctx_cancel_freeze(), I had to set ctx->pause to NULL and after
"do_xlate_actions()" returns for IF_GREATER scenario, had to set ctx->exit
= false.

I hope this is sufficient to have freezing work properly for both the paths.

Thanks
Numan





> In check_check_pkt_len(), the inner ct_clear action kind of mixes up two
>> different checks.  I don't think that check_pkt_len should rely on
>> ct_clear.  I recommend putting some other action inside, if one is
>> needed.
>>
>> There isn't any documentation, but some is needed.
>>
>> Tests are needed.
>>
>
> Ack.
> I didn't work on these as I wanted to get feedback about the approach in
> general.
>
> Thanks
> Numan
>
>
>
>> A NEWS item is appropriate.
>>
>> Thanks,
>>
>> Ben.
>>
>
diff mbox series

Patch

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 9b087f1b0..26dd69dcd 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -855,6 +855,24 @@  enum ovs_nat_attr {
 
 #define OVS_NAT_ATTR_MAX (__OVS_NAT_ATTR_MAX - 1)
 
+/*
+ * enum ovs_check_pkt_len_attr - Attributes for %OVS_ACTION_ATTR_CHECK_PKT_LEN.
+ *
+ * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
+ * @OVS_CHECK_PKT_LEN_ATTR_USERSPACE_COND: u8 comparison condition to send
+ * the packet to userspace. One of OVS_CHECK_PKT_LEN_COND_*.
+ * @OVS_CHECK_PKT_LEN_ATTR_USERPACE - Nested OVS_USERSPACE_ATTR_* actions.
+ */
+enum ovs_check_pkt_len_attr {
+    OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
+    OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
+    OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
+    OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
+    __OVS_CHECK_PKT_LEN_ATTR_MAX,
+};
+
+#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
+
 /**
  * enum ovs_action_attr - Action types.
  *
@@ -909,8 +927,6 @@  enum ovs_nat_attr {
  * tunnel header.
  * @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the
  * packet, or modify the packet (e.g., change the DSCP field).
- * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
- * actions without affecting the original packet and key.
  */
 
 enum ovs_action_attr {
@@ -936,12 +952,13 @@  enum ovs_action_attr {
 	OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
 	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
 	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
-	OVS_ACTION_ATTR_METER,        /* u32 meter number. */
-	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
+	OVS_ACTION_ATTR_METER,         /* u32 meter number. */
+	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
 
 #ifndef __KERNEL__
 	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
 	OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
+	OVS_ACTION_ATTR_CLONE,         /* Nested OVS_CLONE_ATTR_*.  */
 #endif
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
@@ -1034,9 +1051,4 @@  struct ovs_zone_limit {
 	__u32 count;
 };
 
-#define OVS_CLONE_ATTR_EXEC      0   /* Specify an u32 value. When nonzero,
-				      * actions in clone will not change flow
-				      * keys. False otherwise.
-				      */
-
 #endif /* _LINUX_OPENVSWITCH_H */
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 4daf5ad07..7e063afff 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -123,6 +123,8 @@  struct vl_mff_map;
     OFPACT(NAT,             ofpact_nat,         ofpact, "nat")          \
     OFPACT(OUTPUT_TRUNC,    ofpact_output_trunc,ofpact, "output_trunc") \
     OFPACT(CLONE,           ofpact_nest,        actions, "clone")       \
+    OFPACT(CHECK_PKT_LARGER, ofpact_check_pkt_larger, ofpact,           \
+           "check_pkt_larger")                                          \
                                                                         \
     /* Debugging actions.                                               \
      *                                                                  \
@@ -225,6 +227,13 @@  ofpact_last(const struct ofpact *a, const struct ofpact *ofpacts,
     return ofpact_next(a) == ofpact_end(ofpacts, ofpact_len);
 }
 
+static inline bool
+ofpact_remaining_len(const struct ofpact *a, const struct ofpact *ofpacts,
+                     size_t ofpact_len)
+{
+    return ofpact_len - ((uint8_t *)a - (uint8_t *)ofpacts);
+}
+
 static inline const struct ofpact *
 ofpact_find_type_flattened(const struct ofpact *a, enum ofpact_type type,
                            const struct ofpact * const end)
@@ -620,6 +629,15 @@  struct ofpact_meter {
     );
 };
 
+/* OFPACT_CHECK_PKT_LARGER.
+ *
+ * Used for NXAST_CHECK_PKT_LARGER. */
+struct ofpact_check_pkt_larger {
+    struct ofpact ofpact;
+    uint16_t pkt_len;
+    struct mf_subfield dst;
+};
+
 /* OFPACT_WRITE_ACTIONS, OFPACT_CLONE.
  *
  * Used for OFPIT11_WRITE_ACTIONS, NXAST_CLONE. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1564db9c6..931ae95b9 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6814,6 +6814,7 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
     case OVS_ACTION_ATTR_PUSH_NSH:
     case OVS_ACTION_ATTR_POP_NSH:
     case OVS_ACTION_ATTR_CT_CLEAR:
+    case OVS_ACTION_ATTR_CHECK_PKT_LEN:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }
diff --git a/lib/dpif.c b/lib/dpif.c
index e35f11147..8d0e303cc 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1274,6 +1274,7 @@  dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
     case OVS_ACTION_ATTR_POP_NSH:
     case OVS_ACTION_ATTR_CT_CLEAR:
     case OVS_ACTION_ATTR_UNSPEC:
+    case OVS_ACTION_ATTR_CHECK_PKT_LEN:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 3b6890e95..4b6f06a03 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -642,6 +642,64 @@  odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal,
     }
 }
 
+static void
+odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal,
+                          const struct nlattr *action,
+                          odp_execute_cb dp_execute_action)
+{
+    const struct nlattr *a;
+    const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
+    const struct nlattr *subactions = NULL;
+    size_t subactions_size = 0;
+    struct dp_packet_batch pb;
+    size_t left;
+
+    bool is_greater = false;
+    memset(attrs, 0, sizeof(attrs));
+    NL_NESTED_FOR_EACH_UNSAFE (a, left, action) {
+        int type = nl_attr_type(a);
+
+        if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type]) {
+            OVS_NOT_REACHED();
+        }
+        attrs[type] = a;
+    }
+
+    if (left) {
+        OVS_NOT_REACHED();
+    }
+    a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
+    if (!a) {
+        OVS_NOT_REACHED();
+    }
+
+    is_greater = dp_packet_size(packet) > nl_attr_get_u16(a);
+    if (is_greater) {
+        a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
+    } else {
+        a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
+    }
+
+    if (a) {
+        subactions =  nl_attr_get(a);
+        subactions_size = nl_attr_get_size(a);
+    }
+
+    if (!steal) {
+        /* The 'subactions' may modify the packet, but the modification
+         * should not propagate beyond this action. Make a copy
+         * the packet in case we don't own the packet, so that the
+         * 'subactions' are only applid to check_pkt_len. 'odp_execute_actions'
+         * will free the clone.  */
+        packet = dp_packet_clone(packet);
+    }
+    /* If subactions is NULL, the packet will be freed by
+     * odp_execute_actions. */
+    dp_packet_batch_init_packet(&pb, packet);
+    odp_execute_actions(dp, &pb, true, subactions, subactions_size,
+                        dp_execute_action);
+}
+
 static bool
 requires_datapath_assistance(const struct nlattr *a)
 {
@@ -673,6 +731,7 @@  requires_datapath_assistance(const struct nlattr *a)
     case OVS_ACTION_ATTR_PUSH_NSH:
     case OVS_ACTION_ATTR_POP_NSH:
     case OVS_ACTION_ATTR_CT_CLEAR:
+    case OVS_ACTION_ATTR_CHECK_PKT_LEN:
         return false;
 
     case OVS_ACTION_ATTR_UNSPEC:
@@ -900,6 +959,19 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
             }
             break;
 
+        case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+            DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+                odp_execute_check_pkt_len(dp, packet, steal && last_action, a,
+                                          dp_execute_action);
+            }
+
+            if (last_action) {
+                /* We do not need to free the packets.
+                 * odp_execute_check_pkt_len() has stolen them. */
+                return;
+            }
+            break;
+
         case OVS_ACTION_ATTR_OUTPUT:
         case OVS_ACTION_ATTR_TUNNEL_PUSH:
         case OVS_ACTION_ATTR_TUNNEL_POP:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index cb6a5f204..d16f7e2a2 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -131,6 +131,7 @@  odp_action_len(uint16_t type)
     case OVS_ACTION_ATTR_CLONE: return ATTR_LEN_VARIABLE;
     case OVS_ACTION_ATTR_PUSH_NSH: return ATTR_LEN_VARIABLE;
     case OVS_ACTION_ATTR_POP_NSH: return 0;
+    case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE;
 
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
@@ -1042,6 +1043,38 @@  format_odp_set_nsh(struct ds *ds, const struct nlattr *attr)
     ds_put_cstr(ds, "))");
 }
 
+static void
+format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr,
+                                const struct hmap *portno_names OVS_UNUSED)
+{
+    static const struct nl_policy ovs_cpl_policy[] = {
+        [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = { .type = NL_A_U16 },
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_cpl_policy)];
+    ds_put_cstr(ds, "check_pkt_len");
+    if (!nl_parse_nested(attr, ovs_cpl_policy, a, ARRAY_SIZE(a))) {
+        ds_put_cstr(ds, "(error)");
+        return;
+    }
+
+    uint16_t pkt_len = nl_attr_get_u16(a[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]);
+    ds_put_format(ds, "(> %u ? (", pkt_len);
+    const struct nlattr *acts;
+    acts = nl_attr_find_nested(attr,
+                               OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
+    if (acts) {
+        format_odp_actions(ds, nl_attr_get(acts), nl_attr_get_size(acts),
+                           portno_names);
+    }
+    ds_put_cstr(ds, ") : (");
+    acts = nl_attr_find_nested(attr,
+                               OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
+    if (acts) {
+        format_odp_actions(ds, nl_attr_get(acts), nl_attr_get_size(acts),
+                           portno_names);
+    }
+    ds_put_cstr(ds, "))");
+}
 
 static void
 format_odp_action(struct ds *ds, const struct nlattr *a,
@@ -1181,6 +1214,9 @@  format_odp_action(struct ds *ds, const struct nlattr *a,
     case OVS_ACTION_ATTR_POP_NSH:
         ds_put_cstr(ds, "pop_nsh()");
         break;
+    case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+        format_odp_check_pkt_len_action(ds, a, portno_names);
+        break;
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
     default:
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 96e39d6c6..7d09be818 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -355,6 +355,8 @@  enum ofp_raw_action_type {
     /* NX1.3+(48): void. */
     NXAST_RAW_DEC_NSH_TTL,
 
+    /* NX1.0+(49): struct nx_action_check_pkt_larger, ... */
+    OFPAT_RAW_CHECK_PKT_LARGER,
 /* ## ------------------ ## */
 /* ## Debugging actions. ## */
 /* ## ------------------ ## */
@@ -492,6 +494,7 @@  ofpact_next_flattened(const struct ofpact *ofpact)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
         return ofpact_next(ofpact);
 
     case OFPACT_CLONE:
@@ -7400,6 +7403,97 @@  check_WRITE_METADATA(const struct ofpact_metadata *a OVS_UNUSED,
     return 0;
 }
 
+/* Check packet length action. */
+
+struct nx_action_check_pkt_larger {
+    ovs_be16 type;              /* OFPAT_VENDOR. */
+    ovs_be16 len;               /* 24. */
+    ovs_be32 vendor;            /* NX_VENDOR_ID. */
+    ovs_be16 subtype;           /* NXAST_OUTPUT_REG. */
+    uint8_t pad[6];
+    ovs_be16 pkt_len;           /* Length of the packet to check. */
+    ovs_be16 ofs_nbits;         /* (ofs << 6) | (n_bits - 1). */
+    ovs_be32 dst;               /* Destination register. */
+};
+
+OFP_ASSERT(sizeof(struct nx_action_check_pkt_larger) == 24);
+
+static enum ofperr
+decode_OFPAT_RAW_CHECK_PKT_LARGER(
+    const struct nx_action_check_pkt_larger *ncpl,
+    enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf *out)
+{
+    struct ofpact_check_pkt_larger *check_pkt_larger =
+        ofpact_put_CHECK_PKT_LARGER(out);
+    check_pkt_larger->pkt_len = ntohs(ncpl->pkt_len);
+    const struct mf_field *mf = mf_from_nxm_header(ntohl(ncpl->dst), NULL);
+    check_pkt_larger->dst.field = mf;
+    check_pkt_larger->dst.ofs = nxm_decode_ofs(ncpl->ofs_nbits);
+    check_pkt_larger->dst.n_bits = nxm_decode_n_bits(ncpl->ofs_nbits);;
+    return 0;
+}
+
+static void
+encode_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *check_pkt_larger,
+                 enum ofp_version ofp_version OVS_UNUSED,
+                 struct ofpbuf *out OVS_UNUSED)
+{
+    struct nx_action_check_pkt_larger *ncpl = put_OFPAT_CHECK_PKT_LARGER(out);
+    ncpl->pkt_len = htons(check_pkt_larger->pkt_len);
+    ncpl->ofs_nbits = nxm_encode_ofs_nbits(
+        check_pkt_larger->dst.ofs, check_pkt_larger->dst.n_bits);
+    if (check_pkt_larger->dst.field) {
+        ncpl->dst = htonl(nxm_header_from_mff(check_pkt_larger->dst.field));
+    }
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+parse_CHECK_PKT_LARGER(char *arg, const struct ofpact_parse_params *pp)
+{
+    char *value;
+    char *delim;
+    char *key;
+    char *error = set_field_split_str(arg, &key, &value, &delim);
+    if (error) {
+        return error;
+    }
+    delim[0] = '\0';
+    if (value[strlen(value) - 1] ==')') {
+        value[strlen(value) - 1] = '\0';
+    }
+    struct mf_subfield dst;
+    error = mf_parse_subfield(&dst, key);
+    if (error) {
+        return error;
+    }
+
+    struct ofpact_check_pkt_larger *check_pkt_larger =
+        ofpact_put_CHECK_PKT_LARGER(pp->ofpacts);
+    error = str_to_u16(value, NULL, &check_pkt_larger->pkt_len);
+    if (error) {
+        return error;
+    }
+    check_pkt_larger->dst = dst;
+    return NULL;
+}
+
+static void
+format_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *a,
+                        const struct ofpact_format_params *fp)
+{
+    ds_put_format(fp->s, "%scheck_pkt_larger(%s%"PRIu32")->",
+                  colors.param, colors.end, a->pkt_len);
+    mf_format_subfield(&a->dst, fp->s);
+}
+
+static enum ofperr
+check_CHECK_PKT_LARGER(const struct ofpact_check_pkt_larger *a OVS_UNUSED,
+                       const struct ofpact_check_params *cp OVS_UNUSED)
+{
+    return 0;
+}
+
+
 /* Goto-Table instruction. */
 
 static void
@@ -7686,6 +7780,7 @@  action_set_classify(const struct ofpact *a)
     case OFPACT_WRITE_METADATA:
     case OFPACT_DEBUG_RECIRC:
     case OFPACT_DEBUG_SLOW:
+    case OFPACT_CHECK_PKT_LARGER:
         return ACTION_SLOT_INVALID;
 
     default:
@@ -7885,6 +7980,7 @@  ovs_instruction_type_from_ofpact_type(enum ofpact_type type)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         return OVSINST_OFPIT11_APPLY_ACTIONS;
     }
@@ -8755,6 +8851,7 @@  ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port)
     case OFPACT_ENCAP:
     case OFPACT_DECAP:
     case OFPACT_DEC_NSH_TTL:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         return false;
     }
@@ -8991,7 +9088,6 @@  ofpacts_parse__(char *str, const struct ofpact_parse_params *pp,
         enum ofpact_type type;
         char *error = NULL;
         ofp_port_t port;
-
         if (ofpact_type_from_name(key, &type)) {
             error = ofpact_parse(type, value, pp);
             inst = ovs_instruction_type_from_ofpact_type(type);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index a8b5a877c..a90b926ef 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -335,6 +335,16 @@  ofputil_parse_key_value(char **stringp, char **keyp, char **valuep)
     char *value = *stringp;
     size_t value_len = parse_value(value, value_delims);
     char value_delim = value[value_len];
+
+    /* Handle the special case if the value is of the form "(x)->y".
+     * After parsing, 'valuep' will be pointing to - "x)->y".
+     * */
+    if (key_delim == '(' && value[value_len] == ')' &&
+        value[value_len + 1] == '-' && value[value_len + 2] == '>') {
+        value_delims = ", \t\r\n";
+        value_len += parse_value(&value[value_len], value_delims);
+        value_delim = value[value_len];
+    }
     value[value_len] = '\0';
     *stringp += value_len + (value_delim != '\0');
 
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 402980678..4b44a773a 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -3014,6 +3014,7 @@  dpif_ipfix_read_actions(const struct flow *flow,
         case OVS_ACTION_ATTR_POP_ETH:
         case OVS_ACTION_ATTR_PUSH_NSH:
         case OVS_ACTION_ATTR_POP_NSH:
+        case OVS_ACTION_ATTR_CHECK_PKT_LEN:
         case OVS_ACTION_ATTR_UNSPEC:
         case __OVS_ACTION_ATTR_MAX:
         default:
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 7da31753c..b21b40bb1 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -1222,6 +1222,7 @@  dpif_sflow_read_actions(const struct flow *flow,
         case OVS_ACTION_ATTR_PUSH_NSH:
         case OVS_ACTION_ATTR_POP_NSH:
         case OVS_ACTION_ATTR_UNSPEC:
+        case OVS_ACTION_ATTR_CHECK_PKT_LEN:
         case __OVS_ACTION_ATTR_MAX:
         default:
             break;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 839fddd99..c0580c359 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5565,6 +5565,7 @@  reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len)
         case OFPACT_UNROLL_XLATE:
         case OFPACT_WRITE_ACTIONS:
         case OFPACT_WRITE_METADATA:
+        case OFPACT_CHECK_PKT_LARGER:
             break;
 
         case OFPACT_CT:
@@ -5873,6 +5874,7 @@  freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
         case OFPACT_CT:
         case OFPACT_CT_CLEAR:
         case OFPACT_NAT:
+        case OFPACT_CHECK_PKT_LARGER:
             /* These may not generate PACKET INs. */
             break;
 
@@ -6065,6 +6067,97 @@  compose_ct_clear_action(struct xlate_ctx *ctx)
     }
 }
 
+static void
+xlate_check_pkt_larger(struct xlate_ctx *ctx,
+                       struct ofpact_check_pkt_larger *check_pkt_larger,
+                       const struct ofpact *remaining_acts,
+                       size_t remaining_acts_len)
+{
+    if (!check_pkt_larger->dst.field) {
+        return;
+    }
+
+    union mf_subvalue value;
+    memset(&value, 0, sizeof value);
+    if (!ctx->xbridge->support.check_pkt_len) {
+        uint8_t is_pkt_larger = 0;
+        if (ctx->xin->packet) {
+            is_pkt_larger =
+                dp_packet_size(ctx->xin->packet) > check_pkt_larger->pkt_len;
+        }
+        value.u8_val = is_pkt_larger;
+        mf_write_subfield_flow(&check_pkt_larger->dst, &value,
+                               &ctx->xin->flow);
+        /* If datapath doesn't support check_pkt_len action, then set the
+         * SLOW_ACTION flag so that ..
+         */
+        ctx->xout->slow |= SLOW_ACTION;
+        return;
+    }
+
+    struct ofpbuf old_stack = ctx->stack;
+    union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
+    ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack);
+    ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size);
+
+    struct ofpbuf old_action_set = ctx->action_set;
+    uint64_t actset_stub[1024 / 8];
+    ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub);
+    ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size);
+
+    struct flow old_flow = ctx->xin->flow;
+    xlate_commit_actions(ctx);
+    struct flow old_base = ctx->base_flow;
+    bool old_was_mpls = ctx->was_mpls;
+    bool old_conntracked = ctx->conntracked;
+
+    size_t offset = nl_msg_start_nested(ctx->odp_actions,
+                                        OVS_ACTION_ATTR_CHECK_PKT_LEN);
+    nl_msg_put_u16(ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
+                    check_pkt_larger->pkt_len);
+    size_t offset_attr = nl_msg_start_nested(
+        ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
+    value.u8_val = 1;
+    mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow);
+    do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false);
+    if (!ctx->freezing) {
+        xlate_action_set(ctx);
+    }
+    if (ctx->freezing) {
+        finish_freezing(ctx);
+    }
+    nl_msg_end_non_empty_nested(ctx->odp_actions, offset_attr);
+
+    ctx->base_flow = old_base;
+    ctx->was_mpls = old_was_mpls;
+    ctx->conntracked = old_conntracked;
+    ctx->xin->flow = old_flow;
+
+    offset_attr = nl_msg_start_nested(
+        ctx->odp_actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
+    value.u8_val = 0;
+    mf_write_subfield_flow(&check_pkt_larger->dst, &value, &ctx->xin->flow);
+    do_xlate_actions(remaining_acts, remaining_acts_len, ctx, true, false);
+    if (!ctx->freezing) {
+        xlate_action_set(ctx);
+    }
+    if (ctx->freezing) {
+        finish_freezing(ctx);
+    }
+    nl_msg_end_non_empty_nested(ctx->odp_actions, offset_attr);
+    nl_msg_end_nested(ctx->odp_actions, offset);
+
+    ofpbuf_uninit(&ctx->action_set);
+    ctx->action_set = old_action_set;
+    ofpbuf_uninit(&ctx->stack);
+    ctx->stack = old_stack;
+    ctx->base_flow = old_base;
+    ctx->was_mpls = old_was_mpls;
+    ctx->conntracked = old_conntracked;
+    ctx->xin->flow = old_flow;
+    ctx->exit = true;
+}
+
 static void
 rewrite_flow_encap_ethernet(struct xlate_ctx *ctx,
                             struct flow *flow,
@@ -6387,6 +6480,7 @@  recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx)
     case OFPACT_WRITE_ACTIONS:
     case OFPACT_WRITE_METADATA:
     case OFPACT_GOTO_TABLE:
+    case OFPACT_CHECK_PKT_LARGER:
     default:
         break;
     }
@@ -6842,6 +6936,21 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_DEBUG_SLOW:
             ctx->xout->slow |= SLOW_ACTION;
             break;
+
+        case OFPACT_CHECK_PKT_LARGER: {
+            if (last) {
+                /* If this is last action, then there is no need to
+                 * translate the action. */
+                break;
+            }
+            const struct ofpact *remaining_acts = ofpact_next(a);
+            size_t remaining_acts_len = ofpact_remaining_len(remaining_acts,
+                                                             ofpacts,
+                                                             ofpacts_len);
+            xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a),
+                                   remaining_acts, remaining_acts_len);
+            break;
+        }
         }
 
         /* Check if need to store this and the remaining actions for later
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2dc5778e1..a34fab89c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1290,6 +1290,52 @@  check_ct_clear(struct dpif_backer *backer)
     return supported;
 }
 
+
+/* Tests whether 'backer''s datapath supports the
+ * OVS_ACTION_ATTR_CHECK_PKT_LEN action. */
+static bool
+check_check_pkt_len(struct dpif_backer *backer)
+{
+    struct odputil_keybuf keybuf;
+    struct ofpbuf actions;
+    struct ofpbuf key;
+    struct flow flow;
+    bool supported;
+
+    struct odp_flow_key_parms odp_parms = {
+        .flow = &flow,
+        .probe = true,
+    };
+
+    memset(&flow, 0, sizeof flow);
+    ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
+    odp_flow_key_from_flow(&odp_parms, &key);
+    ofpbuf_init(&actions, 64);
+    size_t cpl_start;
+    size_t nested_action_start;
+    cpl_start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_CHECK_PKT_LEN);
+    nl_msg_put_u16(&actions, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, 0);
+
+    nested_action_start = nl_msg_start_nested(
+        &actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
+    nl_msg_put_flag(&actions, OVS_ACTION_ATTR_CT_CLEAR);
+    nl_msg_end_nested(&actions, nested_action_start);
+
+    nested_action_start = nl_msg_start_nested(
+        &actions, OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
+    nl_msg_put_flag(&actions, OVS_ACTION_ATTR_CT_CLEAR);
+    nl_msg_end_nested(&actions, nested_action_start);
+    nl_msg_end_nested(&actions, cpl_start);
+
+    supported = dpif_probe_feature(backer->dpif, "check_pkt_len", &key,
+                                   &actions, NULL);
+    ofpbuf_uninit(&actions);
+    VLOG_INFO("%s: Datapath %s check_pkt_len action",
+              dpif_name(backer->dpif), (supported) ? "supports"
+                                                   : "does not support");
+    return supported;
+}
+
 /* Probe the highest dp_hash algorithm supported by the datapath. */
 static size_t
 check_max_dp_hash_alg(struct dpif_backer *backer)
@@ -1397,6 +1443,7 @@  check_support(struct dpif_backer *backer)
     backer->rt_support.ct_eventmask = check_ct_eventmask(backer);
     backer->rt_support.ct_clear = check_ct_clear(backer);
     backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
+    backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
 
     /* Flow fields. */
     backer->rt_support.odp.ct_state = check_ct_state(backer);
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 1a404c82f..cd5321eb9 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -192,7 +192,10 @@  struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
     DPIF_SUPPORT_FIELD(bool, ct_clear, "Conntrack clear")                   \
                                                                             \
     /* Highest supported dp_hash algorithm. */                              \
-    DPIF_SUPPORT_FIELD(size_t, max_hash_alg, "Max dp_hash algorithm")
+    DPIF_SUPPORT_FIELD(size_t, max_hash_alg, "Max dp_hash algorithm")       \
+                                                                            \
+    /* True if the datapath supports OVS_ACTION_ATTR_CHECK_PKT_LEN. */   \
+    DPIF_SUPPORT_FIELD(bool, check_pkt_len, "Check pkt length action")
 
 /* Stores the various features which the corresponding backer supports. */
 struct dpif_backer_support {