[ovs-dev] [PATCH 1/5] conntrack: add commands to r/w conntrack parameters.
Darrell Ball
dball at vmware.com
Tue Sep 19 07:50:21 UTC 2017
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-bounces at openvswitch.org on behalf of antonio.fischetti at intel.com" <ovs-dev-bounces at openvswitch.org on behalf of antonio.fischetti at 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.fischetti at 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’
+ 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 ?
+
+ /* 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.
+ 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 ?
+ 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.
+ if (!valid_param) {
+ VLOG_DBG("%s: expected a valid PARAM", get_param);
[Darrell] Why is ‘PARAM’ capitalized
VLOG_WARN ?
+ 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’ ?
+ 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() ?
+ 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 ?
{ "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
dev at 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=
More information about the dev
mailing list