Thanks Darrell for your comments, I'll re-spin a V2 based on your feedback. Other details inline below.
-Antonio > -----Original Message----- > From: Darrell Ball [mailto:db...@vmware.com] > Sent: Tuesday, September 19, 2017 8:50 AM > To: Fischetti, Antonio <antonio.fische...@intel.com>; d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 1/5] conntrack: add commands to r/w conntrack > parameters. > > Thanks for working on this Antonio > > Few initial comments; in some cases, I did not repeat the same comment. > > > On 9/18/17, 3:22 AM, "ovs-dev-boun...@openvswitch.org on behalf of > antonio.fische...@intel.com" <ovs-dev-boun...@openvswitch.org on behalf of > antonio.fische...@intel.com> wrote: > > Add infrastructure to implement: > - dpctl/ct-get to read a current value of available > conntrack parameters. > - dpctl/ct-set to set a value to the available conntrack > parameters. > > Add dpctl/ct-get to read current values of conntrack > parameters. > Add dpctl/ct-set to set a value to conntrack parameters. > > Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com> > --- > lib/conntrack.c | 67 +++++++++++++++++++++++++++++++++++++++++ > lib/conntrack.h | 3 ++ > lib/ct-dpif.c | 28 ++++++++++++++++++ > lib/ct-dpif.h | 2 ++ > lib/dpctl.c | 85 > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/dpif-netdev.c | 19 ++++++++++++ > lib/dpif-netlink.c | 2 ++ > lib/dpif-provider.h | 4 +++ > 8 files changed, 210 insertions(+) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 419cb1d..0642cc8 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -67,6 +67,13 @@ enum ct_alg_mode { > CT_TFTP_MODE, > }; > > +/* Variable to manage read/write on CT parameters. */ > +struct ct_wk_params { > > [Darrell] > Somehow, I don’t get the ‘_wk_’ > How about ‘cfg’ [Antonio] agree, ct_cfg_params sounds better. I was using 'wk' as 'working'. > > + char *cli; /* Parameter name in human format. */ > + int (*wr)(struct conntrack *, uint32_t); > + int (*rd)(struct conntrack *, uint32_t *); > +}; > + > static bool conn_key_extract(struct conntrack *, struct dp_packet *, > ovs_be16 dl_type, struct conn_lookup_ctx *, > uint16_t zone); > @@ -2391,6 +2398,66 @@ conntrack_flush(struct conntrack *ct, const > uint16_t > *zone) > return 0; > } > > +/* List of parameters that can be read/written at run-time. */ > +struct ct_wk_params wk_params[] = {}; > + > +int > +conntrack_set_param(struct conntrack *ct, > + const char *set_param) > +{ > + bool valid_param = false; > + uint32_t max_conn; > + char bfr[16] = ""; > > [Darrell] > bfr ? > could we use a few more letters ? [Antonio] As it is a temp buffer I could rename it 'temp' or 'buf'? > > + > + /* Check if the specified param can be managed. */ > + for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); > i++) { > + if (!strncmp(set_param, wk_params[i].cli, > + strlen(wk_params[i].cli))) { > + valid_param = true; > + ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1); > + strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr)); > + if (ovs_scan(set_param, bfr, &max_conn)) { > + return (wk_params[i].wr > + ? wk_params[i].wr(ct, max_conn) > + : EOPNOTSUPP); > + } else { > + return EINVAL; > + } > + } > + } > > [Darrell] If we reach here, then won’t valid_param be false since we otherwise > returned. [Antonio] you're right, will fix. > > > + if (!valid_param) { > + VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param); > > [Darrell] VLOG_WARN ? > ‘PARAM=NUMBER’ capitalization ? > Could we use a sentence or sentence fragment ? [Antonio] will change it to VLOG_WARN("%s parameter is not managed by this command.", set_param); > > + return EINVAL; > + } > + > + return 0; > +} > + > +int > +conntrack_get_param(struct conntrack *ct, > + const char *get_param, uint32_t *val) > +{ > + bool valid_param = false; > + > + /* Check if the specified param can be managed. */ > + for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); > i++) { > + if (!strncmp(get_param, wk_params[i].cli, > + strlen(wk_params[i].cli))) { > + valid_param = true; > + > + return (wk_params[i].rd > + ? wk_params[i].rd(ct, val) > + : EOPNOTSUPP); > + } > + } > > [Darrell] If we reach here, then won’t valid_param be false since we otherwise > returned. [Antonio] right, will fix. > > + if (!valid_param) { > + VLOG_DBG("%s: expected a valid PARAM", get_param); > > [Darrell] Why is ‘PARAM’ capitalized > VLOG_WARN ? [Antonio] will change it to VLOG_WARN("%s parameter is not managed by this command.", get_param); > > > + return EINVAL; > + } > + > + return 0; > +} > + > /* This function must be called with the ct->resources read lock taken. > */ > static struct alg_exp_node * > expectation_lookup(struct hmap *alg_expectations, > diff --git a/lib/conntrack.h b/lib/conntrack.h > index fbeef1c..4eb9a9a 100644 > --- a/lib/conntrack.h > +++ b/lib/conntrack.h > @@ -114,6 +114,9 @@ int conntrack_dump_next(struct conntrack_dump *, > struct > ct_dpif_entry *); > int conntrack_dump_done(struct conntrack_dump *); > > int conntrack_flush(struct conntrack *, const uint16_t *zone); > +int conntrack_set_param(struct conntrack *, const char *set_param); > +int conntrack_get_param(struct conntrack *, const char *get_param, > + uint32_t *val); > ? > /* 'struct ct_lock' is a wrapper for an adaptive mutex. It's useful to > try > * different types of locks (e.g. spinlocks) */ > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index c79e69e..599bc57 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -127,6 +127,34 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t > *zone) > : EOPNOTSUPP); > } > > +int > +ct_dpif_set_param(struct dpif *dpif, const char *set_param) > +{ > + if (!set_param) { > + VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif)); > + return EINVAL; > + } > + VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param); > + > + return (dpif->dpif_class->ct_set_param > + ? dpif->dpif_class->ct_set_param(dpif, set_param) > + : EOPNOTSUPP); > +} > + > +int > +ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t > *val) > +{ > + if (!get_param) { > + VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif)); > + return EINVAL; > + } > + VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param); > + > + return (dpif->dpif_class->ct_get_param > + ? dpif->dpif_class->ct_get_param(dpif, get_param, val) > + : EOPNOTSUPP); > +} > + > void > ct_dpif_entry_uninit(struct ct_dpif_entry *entry) > { > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h > index d5f9661..92ce3e9 100644 > --- a/lib/ct-dpif.h > +++ b/lib/ct-dpif.h > @@ -196,6 +196,8 @@ int ct_dpif_dump_start(struct dpif *, struct > ct_dpif_dump_state **, > int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry > *); > int ct_dpif_dump_done(struct ct_dpif_dump_state *); > int ct_dpif_flush(struct dpif *, const uint16_t *zone); > +int ct_dpif_set_param(struct dpif *dpif, const char *set_param); > +int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t > *val); > 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 8951d6e..2a4a924 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -1563,6 +1563,89 @@ dpctl_ct_bkts(int argc, const char *argv[], > free(conn_per_bkts); > return error; > } > + > +static int > +dpctl_ct_set_param(int argc, const char *argv[], > + struct dpctl_params *dpctl_p) > +{ > + struct dpif *dpif; > + char *name; > + int error; > + > + name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p); > > [Darrell] Maybe a comment ? > enum for ‘3’ ? [Antonio] agree, will add a comment also on other lines where this 3 is appearing. > > + if (!name) { > + return EINVAL; > + } > + error = parsed_dpif_open(name, false, &dpif); > + free(name); > + if (error) { > + dpctl_error(dpctl_p, error, "opening datapath"); > + return error; > + } > + > + error = ct_dpif_set_param(dpif, argv[argc - 1]); > + > + switch (error) { > > [Darrell] ovs_strerror() ? [Antonio] ok will change. > > + case 0: > + dpctl_print(dpctl_p, "Ok"); > + break; > + case EOPNOTSUPP: > + dpctl_print(dpctl_p, "Error:%d operation not supported.", error); > + break; > + case EINVAL: > + dpctl_print(dpctl_p, "Error:%d invalid parameter.", error); > + break; > + default: > + dpctl_print(dpctl_p, "Error:%d", error); > + break; > + } > + dpif_close(dpif); > + > + return error; > +} > + > +static int > +dpctl_ct_get_param(int argc, const char *argv[], > + struct dpctl_params *dpctl_p) > +{ > + struct dpif *dpif; > + uint32_t param_val; > + char *name; > + int error; > + > + name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p); > + if (!name) { > + return EINVAL; > + } > + error = parsed_dpif_open(name, false, &dpif); > + free(name); > + if (error) { > + dpctl_error(dpctl_p, error, "opening datapath"); > + return error; > + } > + > + error = ct_dpif_get_param(dpif, argv[argc - 1], ¶m_val); > + > + switch (error) { > + case 0: > + dpctl_print(dpctl_p, "Current value: %d", param_val); > + break; > + case EOPNOTSUPP: > + dpctl_print(dpctl_p, "Error:%d operation not supported.", error); > + break; > + case EINVAL: > + dpctl_print(dpctl_p, "Error:%d invalid parameter.", error); > + break; > + default: > + dpctl_print(dpctl_p, "Error:%d", error); > + break; > + } > + > + dpif_close(dpif); > + > + return error; > +} > + > ? > /* Undocumented commands for unit testing. */ > > @@ -1859,6 +1942,8 @@ static const struct dpctl_command all_commands[] = { > { "ct-stats-show", "[dp] [zone=N] [verbose]", > 0, 3, dpctl_ct_stats_show, DP_RO }, > { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO }, > + { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW }, > + { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO }, > > [Darrell] > ct-set-glbl-cfg ? > ct-get-glbl-cfg ? [Antonio] ok will change > > > { "help", "", 0, INT_MAX, dpctl_help, DP_RO }, > { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO }, > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index ca74df8..8fda2a9 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -5689,6 +5689,23 @@ dpif_netdev_ct_flush(struct dpif *dpif, const > uint16_t *zone) > return conntrack_flush(&dp->conntrack, zone); > } > > +static int > +dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param) > +{ > + struct dp_netdev *dp = get_dp_netdev(dpif); > + > + return conntrack_set_param(&dp->conntrack, set_param); > +} > + > +static int > +dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param, > + uint32_t *val) > +{ > + struct dp_netdev *dp = get_dp_netdev(dpif); > + > + return conntrack_get_param(&dp->conntrack, get_param, val); > +} > + > const struct dpif_class dpif_netdev_class = { > "netdev", > dpif_netdev_init, > @@ -5734,6 +5751,8 @@ const struct dpif_class dpif_netdev_class = { > dpif_netdev_ct_dump_next, > dpif_netdev_ct_dump_done, > dpif_netdev_ct_flush, > + dpif_netdev_ct_set_param, > + dpif_netdev_ct_get_param, > 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 29001fb..0945fad 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -2986,6 +2986,8 @@ const struct dpif_class dpif_netlink_class = { > dpif_netlink_ct_dump_next, > dpif_netlink_ct_dump_done, > dpif_netlink_ct_flush, > + NULL, /* ct_set_param */ > + NULL, /* ct_get_param */ > 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 1d82a09..262b2e0 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -427,6 +427,10 @@ struct dpif_class { > /* Flushes the connection tracking tables. If 'zone' is not NULL, > * only deletes connections in '*zone'. */ > int (*ct_flush)(struct dpif *, const uint16_t *zone); > + /* Set a value to a connection tracking working parameter. */ > + int (*ct_set_param)(struct dpif *, const char *set_param); > + /* Read the current value of a connection tracking working parameter. > */ > + int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t > *val); > > /* Meters */ > > -- > 2.4.11 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__mail.openvswitch.org_mailman_listinfo_ovs- > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih- > uZnsw&m=LD6cChwv8eq9Z53TD21taZaO8qTUdd29sNOYJkq- > Rxc&s=Vi5y2hCa2H_54KZLLC7_hBvH5E3bcpu2916yMH4lzbQ&e= > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
