Sure Kevin, I'll add a cover letter to the new version I'm going to rework. Basically these are two new commands to allow read/adjust some of the CT configuration parameter. So that the user can try to find a better tuning for his/her setup.
Thanks, Antonio > -----Original Message----- > From: Kevin Traynor [mailto:[email protected]] > Sent: Tuesday, September 19, 2017 2:11 PM > To: Fischetti, Antonio <[email protected]>; [email protected] > Subject: Re: [ovs-dev] [PATCH 1/5] conntrack: add commands to r/w conntrack > parameters. > > On 09/18/2017 11:21 AM, [email protected] 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. > > > > Hi Antonio - The commit message doesn't tell why these are needed or > what use cases they will help with etc. Can you add something in a cover > letter or the commits? > > thanks, > Kevin. > > > Signed-off-by: Antonio Fischetti <[email protected]> > > --- > > 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 { > > + 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] = ""; > > + > > + /* 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; > > + } > > + } > > + } > > + if (!valid_param) { > > + VLOG_DBG("%s: expected valid PARAM=NUMBER", 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); > > + } > > + } > > + if (!valid_param) { > > + VLOG_DBG("%s: expected a valid PARAM", 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); > > + 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) { > > + 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 }, > > { "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 */ > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
