| Message ID | 1523242444-76467-6-git-send-email-dlu998@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Justin Pettit |
| Headers | show |
| Series | Userspace datapath: Add fragmentation support. | expand |
> On Apr 8, 2018, at 7:53 PM, Darrell Ball <dlu998@gmail.com> wrote: > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index 5fa3a97..32d55c1 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -164,6 +164,14 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns) > : EOPNOTSUPP); > } > > +int > +ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable) > +{ > + return (dpif->dpif_class->ipf_change_enabled > + ? dpif->dpif_class->ipf_change_enabled(dpif, v6, enable) > + : EOPNOTSUPP); > +} The command is "ipf-set-enabled", and I think that would be a good name for the function, too. It's a bit shorter, and it follows the pattern of other dpif functions. > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 47f4182..9fc0151 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -35,6 +35,7 @@ > > ... > +static int > +dpctl_ct_ipf_change_enabled(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 enabled; > + if (ovs_scan(argv[argc - 1], "%"SCNu32, &enabled)) { > + error = ct_dpif_ipf_change_enabled( > + dpif, !strncmp(v4_or_v6, "v6", 2), enabled); > + if (!error) { > + dpctl_print(dpctl_p, > + "changing fragmentation enabled successful"); > + } else { > + dpctl_error(dpctl_p, error, > + "changing fragmentation enabled failed"); I think these two messages would be confusing if someone were attempted to disable fragmentation. How about putting some code in that prints whether they were enabling or disabling fragmentation reassembly. For example, "enabling fragmentation reassembly successful" or "disabling fragmentation reassembly successful". > + } > + } else { > + error = EINVAL; > + dpctl_error( > + dpctl_p, error, > + "parameter missing: 0 for disabled or 1 for enabled"); > + } > + } else { > + error = EINVAL; > + dpctl_error(dpctl_p, error, > + "parameter missing: v4 for ipv4 or v6 for ipv6"); I would quote "v4" and "v6" to make it clear exactly what arguments were required. > diff --git a/lib/dpctl.man b/lib/dpctl.man > index 9e9d2dc..9bf489c 100644 > --- a/lib/dpctl.man > +++ b/lib/dpctl.man > @@ -268,3 +268,14 @@ Only supported for userspace datapath. > \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] > Read the current number of connection tracker connections. > Only supported for userspace datapath. > +. > +.TP > +\*(DX\fBipf\-set\-enabled\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBenable\fR I think you want to us "\fB" for "v4" and "v6", since they're constants, and you should drop the brackets since the argument isn't optional. Also, the "or" shouldn't be included, and I think using "|" is more common. I think you want something like the following: "\fBv4\fR | \fBv6\fR". It seems like the argument is 0 or 1, and not "enable". How about exposing separate commands for enable and disable? I think it would be clearer, and the internal implementation could remain the same passing bool arguments. > +Enables or disables fragmentation handling for the userspace datapath > +connection tracker. Either v4 or v6 must be specified. Both v4 and v6 These references to "v4" and "v6" should be bolded. > +are enabled by default. When fragmentation handling is enabled, the I'd mention that they're default "on" at the end. > +rules for handling fragments before entering conntrack should not > +differentiate between first and other fragments. Although, this would > +logically already be true anyways, it is mentioned for clarity. If there I would drop the "Although" sentence, since it doesn't add anything. > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 62b3598..08e0944 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -444,6 +444,8 @@ struct dpif_class { > /* Get number of connections tracked. */ > int (*ct_get_nconns)(struct dpif *, uint32_t *nconns); > > + /* IP Fragmentation. */ > + int (*ipf_change_enabled)(struct dpif *, bool, bool); > /* Meters */ I would put a blank line before "/* Meters */" to break them into sections. > diff --git a/lib/ipf.c b/lib/ipf.c > index 3837c60..54f27d2 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -1236,3 +1236,18 @@ ipf_destroy(void) > ipf_lock_unlock(&ipf_lock); > ipf_lock_destroy(&ipf_lock); > } > + > +int > +ipf_change_enabled(bool v6, bool enable) All the other arguments in "lib/ipf.c" are 'v4'. How about using it here, too, instead of 'v6'? (And propagating it up through the call stack.) > +{ > + if ((v6 != true && v6 != false) || > + (enable != true && enable != false)) { > + return 1; > + } If they're bools, how would they be anything be true or false? > diff --git a/lib/ipf.h b/lib/ipf.h > index 5861e96..0b45de9 100644 > --- a/lib/ipf.h > +++ b/lib/ipf.h > @@ -60,4 +60,7 @@ ipf_init(void); > void > ipf_destroy(void); > > +int > +ipf_change_enabled(bool v6, bool enable); According to the style guide, the return type and function name should be on the same line. Thanks, --Justin
Thanks for the detailed review Justin On Tue, Jun 5, 2018 at 11:36 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Apr 8, 2018, at 7:53 PM, Darrell Ball <dlu998@gmail.com> wrote: > > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index 5fa3a97..32d55c1 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -164,6 +164,14 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t > *nconns) > > : EOPNOTSUPP); > > } > > > > +int > > +ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable) > > +{ > > + return (dpif->dpif_class->ipf_change_enabled > > + ? dpif->dpif_class->ipf_change_enabled(dpif, v6, enable) > > + : EOPNOTSUPP); > > +} > > The command is "ipf-set-enabled", and I think that would be a good name > for the function, too. It's a bit shorter, and it follows the pattern of > other dpif functions. > yeah, originally I was using the same name locally and I changed it to this abysmal name; not sure why. > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 47f4182..9fc0151 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -35,6 +35,7 @@ > > > > ... > > +static int > > +dpctl_ct_ipf_change_enabled(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 enabled; > > + if (ovs_scan(argv[argc - 1], "%"SCNu32, &enabled)) { > > + error = ct_dpif_ipf_change_enabled( > > + dpif, !strncmp(v4_or_v6, "v6", 2), enabled); > > + if (!error) { > > + dpctl_print(dpctl_p, > > + "changing fragmentation enabled > successful"); > > + } else { > > + dpctl_error(dpctl_p, error, > > + "changing fragmentation enabled > failed"); > > I think these two messages would be confusing if someone were attempted to > disable fragmentation. How about putting some code in that prints whether > they were enabling or disabling fragmentation reassembly. For example, > "enabling fragmentation reassembly successful" or "disabling fragmentation > reassembly successful". > yeah, I was lazy and used some prior art error message style. > > > + } > > + } else { > > + error = EINVAL; > > + dpctl_error( > > + dpctl_p, error, > > + "parameter missing: 0 for disabled or 1 for > enabled"); > > + } > > + } else { > > + error = EINVAL; > > + dpctl_error(dpctl_p, error, > > + "parameter missing: v4 for ipv4 or v6 for > ipv6"); > > I would quote "v4" and "v6" to make it clear exactly what arguments were > required. > good point > > > diff --git a/lib/dpctl.man b/lib/dpctl.man > > index 9e9d2dc..9bf489c 100644 > > --- a/lib/dpctl.man > > +++ b/lib/dpctl.man > > @@ -268,3 +268,14 @@ Only supported for userspace datapath. > > \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] > > Read the current number of connection tracker connections. > > Only supported for userspace datapath. > > +. > > +.TP > > +\*(DX\fBipf\-set\-enabled\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBenable\fR > > I think you want to us "\fB" for "v4" and "v6", since they're constants, > and you should drop the brackets since the argument isn't optional. Also, > the "or" shouldn't be included, and I think using "|" is more common. I > think you want something like the following: "\fBv4\fR | \fBv6\fR". > ughhh, did I write "\fIv4 or v6\fR" ? thanks > It seems like the argument is 0 or 1, and not "enable". How about > exposing separate commands for enable and disable? I think it would be > clearer, and the internal implementation could remain the same passing bool > arguments. > > > +Enables or disables fragmentation handling for the userspace datapath > > +connection tracker. Either v4 or v6 must be specified. Both v4 and v6 > > These references to "v4" and "v6" should be bolded. > yep, thanks > > > +are enabled by default. When fragmentation handling is enabled, the > > I'd mention that they're default "on" at the end. > yep; was on my todo list for a brief moment > > > +rules for handling fragments before entering conntrack should not > > +differentiate between first and other fragments. Although, this would > > +logically already be true anyways, it is mentioned for clarity. If > there > > I would drop the "Although" sentence, since it doesn't add anything. > yep; not needed. > > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 62b3598..08e0944 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -444,6 +444,8 @@ struct dpif_class { > > /* Get number of connections tracked. */ > > int (*ct_get_nconns)(struct dpif *, uint32_t *nconns); > > > > + /* IP Fragmentation. */ > > + int (*ipf_change_enabled)(struct dpif *, bool, bool); > > /* Meters */ > > I would put a blank line before "/* Meters */" to break them into sections. > yep > > > diff --git a/lib/ipf.c b/lib/ipf.c > > index 3837c60..54f27d2 100644 > > --- a/lib/ipf.c > > +++ b/lib/ipf.c > > @@ -1236,3 +1236,18 @@ ipf_destroy(void) > > ipf_lock_unlock(&ipf_lock); > > ipf_lock_destroy(&ipf_lock); > > } > > + > > +int > > +ipf_change_enabled(bool v6, bool enable) > > All the other arguments in "lib/ipf.c" are 'v4'. How about using it here, > too, instead of 'v6'? (And propagating it up through the call stack.) > The intention was for at least external interfaces to use "bool v6" parameter (ipf_set_min_frag and ipf_change_enabled) with v4 implied. I intended to go back and change the internal APIs to same, but did not get back to it. I probably will change the internal APIs to v6 parameter. > > > +{ > > + if ((v6 != true && v6 != false) || > > + (enable != true && enable != false)) { > > + return 1; > > + } > > If they're bools, how would they be anything be true or false? > At one point, the API chain from dpctl, including ct_dpif_ipf_change_enabled(), dpif_netdev_ipf_change_enabled() and ipf_change_enabled() were "uint32_t" values and I was doing the range checking at the ipf layer. When I changed the other APIs below dpctl to bool. I intended to move the range checking to dpctl. I just noticed the range check is missing in dpctl and I will move this range check there. > > > diff --git a/lib/ipf.h b/lib/ipf.h > > index 5861e96..0b45de9 100644 > > --- a/lib/ipf.h > > +++ b/lib/ipf.h > > @@ -60,4 +60,7 @@ ipf_init(void); > > void > > ipf_destroy(void); > > > > +int > > +ipf_change_enabled(bool v6, bool enable); > > According to the style guide, the return type and function name should be > on the same line. > thanks > > Thanks, > > --Justin > > >
diff --git a/NEWS b/NEWS index 2f31680..8e5a9ad 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,8 @@ Post-v2.9.0 use --names or --no-names to override. See ovs-ofctl(8) for details. - Userspace datapath: * Add v4/v6 fragmentation support for conntrack. + * New "ovs-appctl dpctl/ipf-set-enabled" 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 5fa3a97..32d55c1 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -164,6 +164,14 @@ ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns) : EOPNOTSUPP); } +int +ct_dpif_ipf_change_enabled(struct dpif *dpif, bool v6, bool enable) +{ + return (dpif->dpif_class->ipf_change_enabled + ? dpif->dpif_class->ipf_change_enabled(dpif, v6, enable) + : EOPNOTSUPP); +} + void ct_dpif_entry_uninit(struct ct_dpif_entry *entry) { diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 09e7698..3f0bce5 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -200,6 +200,7 @@ int ct_dpif_flush(struct dpif *, const uint16_t *zone, 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); 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 47f4182..9fc0151 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -35,6 +35,7 @@ #include "dpif.h" #include "openvswitch/dynamic-string.h" #include "flow.h" +#include "ipf.h" #include "openvswitch/match.h" #include "netdev.h" #include "netdev-dpdk.h" @@ -1747,6 +1748,44 @@ dpctl_ct_get_nconns(int argc, const char *argv[], return error; } +static int +dpctl_ct_ipf_change_enabled(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 enabled; + if (ovs_scan(argv[argc - 1], "%"SCNu32, &enabled)) { + error = ct_dpif_ipf_change_enabled( + dpif, !strncmp(v4_or_v6, "v6", 2), enabled); + if (!error) { + dpctl_print(dpctl_p, + "changing fragmentation enabled successful"); + } else { + dpctl_error(dpctl_p, error, + "changing fragmentation enabled failed"); + } + } else { + error = EINVAL; + dpctl_error( + dpctl_p, error, + "parameter missing: 0 for disabled or 1 for enabled"); + } + } 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 @@ -2046,6 +2085,8 @@ static const struct dpctl_command all_commands[] = { { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, DP_RW }, { "ct-get-maxconns", "[dp]", 0, 1, dpctl_ct_get_maxconns, DP_RO }, { "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 }, { "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 9e9d2dc..9bf489c 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -268,3 +268,14 @@ Only supported for userspace datapath. \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] Read the current number of connection tracker connections. Only supported for userspace datapath. +. +.TP +\*(DX\fBipf\-set\-enabled\fR [\fIdp\fR] [\fIv4 or v6\fR] \fBenable\fR +Enables or disables fragmentation handling for the userspace datapath +connection tracker. Either v4 or v6 must be specified. Both v4 and v6 +are enabled by default. When fragmentation handling is enabled, the +rules for handling fragments before entering conntrack should not +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. diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index be31fd0..c36110a 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -47,6 +47,7 @@ #include "flow.h" #include "hmapx.h" #include "id-pool.h" +#include "ipf.h" #include "latch.h" #include "netdev.h" #include "netdev-vport.h" @@ -5870,6 +5871,13 @@ dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns) return conntrack_get_nconns(&dp->conntrack, nconns); } +static int +dpif_netdev_ipf_change_enabled(struct dpif *dpif OVS_UNUSED, bool v6, + bool enable) +{ + return ipf_change_enabled(v6, enable); +} + const struct dpif_class dpif_netdev_class = { "netdev", dpif_netdev_init, @@ -5918,6 +5926,7 @@ const struct dpif_class dpif_netdev_class = { dpif_netdev_ct_set_maxconns, dpif_netdev_ct_get_maxconns, dpif_netdev_ct_get_nconns, + dpif_netdev_ipf_change_enabled, 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 17be3dd..cc9e738 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2994,6 +2994,7 @@ const struct dpif_class dpif_netlink_class = { NULL, /* ct_set_maxconns */ NULL, /* ct_get_maxconns */ NULL, /* ct_get_nconns */ + NULL, /* ipf_change_enabled */ 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 62b3598..08e0944 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -444,6 +444,8 @@ struct dpif_class { /* Get number of connections tracked. */ int (*ct_get_nconns)(struct dpif *, uint32_t *nconns); + /* IP Fragmentation. */ + int (*ipf_change_enabled)(struct dpif *, bool, bool); /* Meters */ /* Queries 'dpif' for supported meter features. diff --git a/lib/ipf.c b/lib/ipf.c index 3837c60..54f27d2 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -1236,3 +1236,18 @@ ipf_destroy(void) ipf_lock_unlock(&ipf_lock); ipf_lock_destroy(&ipf_lock); } + +int +ipf_change_enabled(bool v6, bool enable) +{ + if ((v6 != true && v6 != false) || + (enable != true && enable != false)) { + return 1; + } + if (v6) { + atomic_store_relaxed(&ifp_v6_enabled, enable); + } else { + atomic_store_relaxed(&ifp_v4_enabled, enable); + } + return 0; +} diff --git a/lib/ipf.h b/lib/ipf.h index 5861e96..0b45de9 100644 --- a/lib/ipf.h +++ b/lib/ipf.h @@ -60,4 +60,7 @@ ipf_init(void); void ipf_destroy(void); +int +ipf_change_enabled(bool v6, bool enable); + #endif /* ipf.h */
Signed-off-by: Darrell Ball <dlu998@gmail.com> --- NEWS | 2 ++ lib/ct-dpif.c | 8 ++++++++ lib/ct-dpif.h | 1 + lib/dpctl.c | 41 +++++++++++++++++++++++++++++++++++++++++ lib/dpctl.man | 11 +++++++++++ lib/dpif-netdev.c | 9 +++++++++ lib/dpif-netlink.c | 1 + lib/dpif-provider.h | 2 ++ lib/ipf.c | 15 +++++++++++++++ lib/ipf.h | 3 +++ 10 files changed, 93 insertions(+)