[ovs-dev,v6,06/10] ipf: Add set minimum fragment size command.

Message ID 1523242444-76467-7-git-send-email-dlu998@gmail.com
State Changes Requested
Delegated to: Justin Pettit
Headers show
Series
  • Userspace datapath: Add fragmentation support.
Related show

Commit Message

Darrell Ball April 9, 2018, 2:54 a.m.
A new command "ovs-appctl dpctl/ipf-set-minfrag" is added
for userspace datapath conntrack fragmentation support.

Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 NEWS                |  2 ++
 lib/ct-dpif.c       |  8 ++++++++
 lib/ct-dpif.h       |  1 +
 lib/dpctl.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 lib/dpctl.man       |  7 +++++++
 lib/dpif-netdev.c   |  8 ++++++++
 lib/dpif-netlink.c  |  1 +
 lib/dpif-provider.h |  2 ++
 lib/ipf.c           | 23 +++++++++++++++++++++++
 lib/ipf.h           |  3 +++
 10 files changed, 95 insertions(+)

Comments

Justin Pettit June 7, 2018, 5 a.m. | #1
> On Apr 8, 2018, at 7:54 PM, Darrell Ball <dlu998@gmail.com> wrote:

> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 9fc0151..f6c0a87 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1786,6 +1786,44 @@ dpctl_ct_ipf_change_enabled(int argc, const char *argv[],
>     return error;
> }
> 
> +static int
> +dpctl_ct_ipf_set_min_frag(int argc, const char *argv[],
> +                          struct dpctl_params *dpctl_p)
> +{
> ...
> +                if (!error) {
> +                    dpctl_print(dpctl_p,
> +                                "setting minimum fragment size successful");
> +                } else {
> +                    dpctl_error(dpctl_p, error,
> +                                "setting minimum fragment size failed");
> +                }

It might be nice to give users an indication of why this failed.  It looks like that will only happen if the value specified isn't valid, so it may be worth just saying that much.

> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 9bf489c..6223c15 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -279,3 +279,10 @@ differentiate between first and other fragments.  Although, this would
> logically already be true anyways, it is mentioned for clarity.  If there
> is a need to differentiate between first and other fragments, do it after
> conntrack.
> +.
> +.TP
> +\*(DX\fBipf\-set\-minfrag\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBminfrag\fR
> +Sets the minimum fragment size supported by the userspace datapath
> +connection tracker.  Either v4 or v6 must be specified.  The default v4
> +value is 1200 and the clamped minimum is 400.  The default v6 value is
> +1280, which is also the clamped minimum.

I think it would be worth explaining a bit more about this parameter.  Can you explain the difference between the value being set here and the clamped value?

I like the name of the function, so what about calling the command "ipf-set-min-frag"?

For all of these functions, it may be worth mentioning that they only apply to the userspace datapath.

> 
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 08e0944..aa9c490 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -446,6 +446,8 @@ struct dpif_class {
> 
>     /* IP Fragmentation. */
>     int (*ipf_change_enabled)(struct dpif *, bool, bool);
> +    /* Set minimum fragment allowed. */
> +    int (*ipf_set_min_frag)(struct dpif *, bool, uint32_t);

For all these definitions, I think it would be worth adding the argument names so the prototypes are a bit clearer.

> 
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 54f27d2..24d9b06 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1251,3 +1251,26 @@ ipf_change_enabled(bool v6, bool enable)
>     }
>     return 0;
> }
> +
> +int
> +ipf_set_min_frag(bool v6, uint32_t value)
> +{
> +    /* If the user specifies an unreasonably large number, fragmentation
> +     * will not work well but it will not blow up. */

It won't blow up, but won't it drop fragments from legitimate IP stacks?

I didn't call them out, but some of my comments on the disabling fragmentation handling patches could apply here, too.

Thanks,

--Justin
Darrell Ball July 9, 2018, 4:56 p.m. | #2
Thanks for the detailed review Justin



On Wed, Jun 6, 2018 at 10:00 PM, Justin Pettit <jpettit@ovn.org> wrote:

>
> > On Apr 8, 2018, at 7:54 PM, Darrell Ball <dlu998@gmail.com> wrote:
>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 9fc0151..f6c0a87 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -1786,6 +1786,44 @@ dpctl_ct_ipf_change_enabled(int argc, const char
> *argv[],
> >     return error;
> > }
> >
> > +static int
> > +dpctl_ct_ipf_set_min_frag(int argc, const char *argv[],
> > +                          struct dpctl_params *dpctl_p)
> > +{
> > ...
> > +                if (!error) {
> > +                    dpctl_print(dpctl_p,
> > +                                "setting minimum fragment size
> successful");
> > +                } else {
> > +                    dpctl_error(dpctl_p, error,
> > +                                "setting minimum fragment size failed");
> > +                }
>
> It might be nice to give users an indication of why this failed.  It looks
> like that will only happen if the value specified isn't valid, so it may be
> worth just saying that much.
>


oops, missed this one; I took the time to add for the other cases but not
here - ADD no doubt.



>
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index 9bf489c..6223c15 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -279,3 +279,10 @@ differentiate between first and other fragments.
> Although, this would
> > logically already be true anyways, it is mentioned for clarity.  If there
> > is a need to differentiate between first and other fragments, do it after
> > conntrack.
> > +.
> > +.TP
> > +\*(DX\fBipf\-set\-minfrag\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBminfrag\fR
> > +Sets the minimum fragment size supported by the userspace datapath
> > +connection tracker.  Either v4 or v6 must be specified.  The default v4
> > +value is 1200 and the clamped minimum is 400.  The default v6 value is
> > +1280, which is also the clamped minimum.
>
> I think it would be worth explaining a bit more about this parameter.  Can
> you explain the difference between the value being set here and the clamped
> value?
>


I added more description



>
> I like the name of the function, so what about calling the command
> "ipf-set-min-frag"?
>


I was trying to avoid the 4-part name, guessing people would not like it; I
prefer the 4-part name myself, so we are
in agreement.



>
> For all of these functions, it may be worth mentioning that they only
> apply to the userspace datapath.
>


All the APIs already say that, so I think we are covered.



>
> >
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 08e0944..aa9c490 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -446,6 +446,8 @@ struct dpif_class {
> >
> >     /* IP Fragmentation. */
> >     int (*ipf_change_enabled)(struct dpif *, bool, bool);
> > +    /* Set minimum fragment allowed. */
> > +    int (*ipf_set_min_frag)(struct dpif *, bool, uint32_t);
>
> For all these definitions, I think it would be worth adding the argument
> names so the prototypes are a bit clearer.
>


yep, this file should follow the practice of using the argument names.



>
> >
> > diff --git a/lib/ipf.c b/lib/ipf.c
> > index 54f27d2..24d9b06 100644
> > --- a/lib/ipf.c
> > +++ b/lib/ipf.c
> > @@ -1251,3 +1251,26 @@ ipf_change_enabled(bool v6, bool enable)
> >     }
> >     return 0;
> > }
> > +
> > +int
> > +ipf_set_min_frag(bool v6, uint32_t value)
> > +{
> > +    /* If the user specifies an unreasonably large number, fragmentation
> > +     * will not work well but it will not blow up. */
>
> It won't blow up, but won't it drop fragments from legitimate IP stacks?
>


I added a comment just to be on the safe side.



>
> I didn't call them out, but some of my comments on the disabling
> fragmentation handling patches could apply here, too.
>


Got it.



>
> Thanks,
>
> --Justin
>
>
>

Patch

diff --git a/NEWS b/NEWS
index 8e5a9ad..13008ba 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@  Post-v2.9.0
      * Add v4/v6 fragmentation support for conntrack.
      * New "ovs-appctl dpctl/ipf-set-enabled" command for userspace datapath
        conntrack fragmentation support.
+     * New "ovs-appctl dpctl/ipf-set-minfragment" command for userspace
+       datapath conntrack fragmentation support.
    - ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
    - OpenFlow:
      * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 32d55c1..4a93bf6 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -172,6 +172,14 @@  ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable)
             : EOPNOTSUPP);
 }
 
+int
+ct_dpif_ipf_set_min_frag(struct dpif *dpif, bool v6, uint32_t min_frag)
+{
+    return (dpif->dpif_class->ipf_set_min_frag
+            ? dpif->dpif_class->ipf_set_min_frag(dpif, v6, min_frag)
+            : EOPNOTSUPP);
+}
+
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 3f0bce5..449f958 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -201,6 +201,7 @@  int ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns);
 int ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns);
 int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
 int ct_dpif_ipf_change_enabled(struct dpif *, bool, bool);
+int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
                           bool verbose, bool print_stats);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 9fc0151..f6c0a87 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1786,6 +1786,44 @@  dpctl_ct_ipf_change_enabled(int argc, const char *argv[],
     return error;
 }
 
+static int
+dpctl_ct_ipf_set_min_frag(int argc, const char *argv[],
+                          struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif, 4);
+    if (!error) {
+        char v4_or_v6[3] = {0};
+        if (ovs_scan(argv[argc - 2], "%2s", v4_or_v6) &&
+            (!strncmp(v4_or_v6, "v4", 2) || !strncmp(v4_or_v6, "v6", 2))) {
+            uint32_t min_fragment;
+            if (ovs_scan(argv[argc - 1], "%"SCNu32, &min_fragment)) {
+                error = ct_dpif_ipf_set_min_frag(
+                            dpif, !strncmp(v4_or_v6, "v6", 2), min_fragment);
+
+                if (!error) {
+                    dpctl_print(dpctl_p,
+                                "setting minimum fragment size successful");
+                } else {
+                    dpctl_error(dpctl_p, error,
+                                "setting minimum fragment size failed");
+                }
+            } else {
+                error = EINVAL;
+                dpctl_error(dpctl_p, error,
+                            "parameter missing for minimum fragment size");
+            }
+        } else {
+            error = EINVAL;
+            dpctl_error(dpctl_p, error,
+                        "parameter missing: v4 for ipv4 or v6 for ipv6");
+        }
+        dpif_close(dpif);
+    }
+
+    return error;
+}
+
 /* Undocumented commands for unit testing. */
 
 static int
@@ -2087,6 +2125,8 @@  static const struct dpctl_command all_commands[] = {
     { "ct-get-nconns", "[dp]", 0, 1, dpctl_ct_get_nconns, DP_RO },
     { "ipf-set-enabled", "[dp] v4_or_v6 enabled", 2, 3,
        dpctl_ct_ipf_change_enabled, DP_RW },
+    { "ipf-set-minfragment", "[dp] v4_or_v6 minfragment", 2, 3,
+       dpctl_ct_ipf_set_min_frag, DP_RW },
     { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
     { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
 
diff --git a/lib/dpctl.man b/lib/dpctl.man
index 9bf489c..6223c15 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -279,3 +279,10 @@  differentiate between first and other fragments.  Although, this would
 logically already be true anyways, it is mentioned for clarity.  If there
 is a need to differentiate between first and other fragments, do it after
 conntrack.
+.
+.TP
+\*(DX\fBipf\-set\-minfrag\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBminfrag\fR
+Sets the minimum fragment size supported by the userspace datapath
+connection tracker.  Either v4 or v6 must be specified.  The default v4
+value is 1200 and the clamped minimum is 400.  The default v6 value is
+1280, which is also the clamped minimum.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c36110a..f51fe52 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5878,6 +5878,13 @@  dpif_netdev_ipf_change_enabled(struct dpif *dpif OVS_UNUSED, bool v6,
     return ipf_change_enabled(v6, enable);
 }
 
+static int
+dpif_netdev_ipf_set_min_frag(struct dpif *dpif OVS_UNUSED, bool v6,
+                             uint32_t min_frag)
+{
+    return ipf_set_min_frag(v6, min_frag);
+}
+
 const struct dpif_class dpif_netdev_class = {
     "netdev",
     dpif_netdev_init,
@@ -5927,6 +5934,7 @@  const struct dpif_class dpif_netdev_class = {
     dpif_netdev_ct_get_maxconns,
     dpif_netdev_ct_get_nconns,
     dpif_netdev_ipf_change_enabled,
+    dpif_netdev_ipf_set_min_frag,
     dpif_netdev_meter_get_features,
     dpif_netdev_meter_set,
     dpif_netdev_meter_get,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index cc9e738..d9333ed 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2995,6 +2995,7 @@  const struct dpif_class dpif_netlink_class = {
     NULL,                       /* ct_get_maxconns */
     NULL,                       /* ct_get_nconns */
     NULL,                       /* ipf_change_enabled */
+    NULL,                       /* ipf_set_min_frag */
     dpif_netlink_meter_get_features,
     dpif_netlink_meter_set,
     dpif_netlink_meter_get,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 08e0944..aa9c490 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -446,6 +446,8 @@  struct dpif_class {
 
     /* IP Fragmentation. */
     int (*ipf_change_enabled)(struct dpif *, bool, bool);
+    /* Set minimum fragment allowed. */
+    int (*ipf_set_min_frag)(struct dpif *, bool, uint32_t);
     /* Meters */
 
     /* Queries 'dpif' for supported meter features.
diff --git a/lib/ipf.c b/lib/ipf.c
index 54f27d2..24d9b06 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -1251,3 +1251,26 @@  ipf_change_enabled(bool v6, bool enable)
     }
     return 0;
 }
+
+int
+ipf_set_min_frag(bool v6, uint32_t value)
+{
+    /* If the user specifies an unreasonably large number, fragmentation
+     * will not work well but it will not blow up. */
+    if ((!v6 && value < IPF_V4_FRAG_SIZE_LBOUND) ||
+        (v6 && value < IPF_V6_FRAG_SIZE_LBOUND)) {
+        return 1;
+    }
+
+    ipf_lock_lock(&ipf_lock);
+    if (v6) {
+        atomic_store_relaxed(&min_v6_frag_size, value);
+    } else {
+        atomic_store_relaxed(&min_v4_frag_size, value);
+        max_v4_frag_list_size = DIV_ROUND_UP(
+            IPV4_PACKET_MAX_SIZE - IPV4_PACKET_MAX_HDR_SIZE,
+            min_v4_frag_size - IPV4_PACKET_MAX_HDR_SIZE);
+    }
+    ipf_lock_unlock(&ipf_lock);
+    return 0;
+}
diff --git a/lib/ipf.h b/lib/ipf.h
index 0b45de9..277852d 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -63,4 +63,7 @@  ipf_destroy(void);
 int
 ipf_change_enabled(bool v6, bool enable);
 
+int
+ipf_set_min_frag(bool v6, uint32_t value);
+
 #endif /* ipf.h */