diff mbox series

[ovs-dev,v3,2/5] conntrack: add commands to r/w CT parameters.

Message ID 1507883117-24347-3-git-send-email-antonio.fischetti@intel.com
State Changes Requested
Delegated to: Darrell Ball
Headers show
Series Conntrack: add commands to r/w CT parameters. | expand

Commit Message

Fischetti, Antonio Oct. 13, 2017, 8:25 a.m. UTC 
Add infrastructure to implement:
 - dpctl/ct-get-glbl-cfg to read a current value of available
   conntrack parameters.
 - dpctl/ct-set-glbl-cfg to set a value to the available conntrack
   parameters.

CC: Darrell Ball <dlu998@gmail.com>
CC: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/conntrack.c     | 60 ++++++++++++++++++++++++++++++++++++++++++
 lib/conntrack.h     |  3 +++
 lib/ct-dpif.c       | 28 ++++++++++++++++++++
 lib/ct-dpif.h       |  2 ++
 lib/dpctl.c         | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/dpif-netdev.c   | 19 ++++++++++++++
 lib/dpif-netlink.c  |  2 ++
 lib/dpif-provider.h |  4 +++
 8 files changed, 194 insertions(+)

Comments

Darrell Ball Dec. 11, 2017, 4:35 p.m. UTC | #1 
Thanks Antonio for doing all this and pushing it forward.

Regarding patches 2-4:

I understand you want to save some code for various possible set and get operations.
The prior art for these commands is however not generic set and get commands.
Sometimes, we have specific commands that can take different numbers of arguments but
those are specific and the context is clear.
If we take some examples, we might see there is an issue with the approach here.
Take per zone limits as one example, which is an internal requirement that we are working on
across datapaths.
This is a set operation with 2 arguments plus the datapath = 3 arguments.
This won’t work with the generic set functions here.
The corresponding get will not work either.
Trying to make this work generically will get pretty messy and the parameter validation error prone.

I would like to propose an alternative (a simple one; also with code consolidation and more error checking) below:
Please let me know what you think.

Thanks Darrell

///////////////////////////////////////////////////////////////

diff --git a/lib/conntrack.c b/lib/conntrack.c
index f5a3aa9..c8ad548 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2485,6 +2485,27 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
     return 0;
 }
 
+int
+conntrack_set_maxconns(struct conntrack *ct, uint32_t maxconns)
+{
+    atomic_init(&ct->n_conn_limit, maxconns);
+    return 0;
+}
+
+int
+conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns)
+{
+    atomic_read_relaxed(&ct->n_conn_limit, maxconns);
+    return 0;
+}
+
+int
+conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns)
+{
+    *nconns = atomic_count_get(&ct->n_conn);
+    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..8652724 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_maxconns(struct conntrack *ct, uint32_t maxconns);
+int conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns);
+int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns);
 ^L
 /* '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 239c848..5fa3a97 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -140,6 +140,30 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone,
             : EOPNOTSUPP);
 }
 
+int
+ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns)
+{
+    return (dpif->dpif_class->ct_set_maxconns
+            ? dpif->dpif_class->ct_set_maxconns(dpif, maxconns)
+            : EOPNOTSUPP);
+}
+
+int
+ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns)
+{
+    return (dpif->dpif_class->ct_get_maxconns
+            ? dpif->dpif_class->ct_get_maxconns(dpif, maxconns)
+            : EOPNOTSUPP);
+}
+
+int
+ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)
+{
+    return (dpif->dpif_class->ct_get_nconns
+            ? dpif->dpif_class->ct_get_nconns(dpif, nconns)
+            : EOPNOTSUPP);
+}
+
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 5e2de53..09e7698 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -197,6 +197,9 @@ 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,
                   const struct ct_dpif_tuple *);
+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);
 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 a28ded9..215d1c3 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1654,6 +1654,96 @@ dpctl_ct_bkts(int argc, const char *argv[],
     return error;
 }
 ^L
+static int
+dpctl_ct_open_dp(int argc, const char *argv[],
+                 struct dpctl_params *dpctl_p, struct dpif **dpif)
+{
+    int error = 0;
+    /* The datapath name is not a mandatory parameter for this command.
+     * If it is not specified - so argc < 3 - we retrieve it from the
+     * current setup, assuming only one exists. */
+    char *dpname = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
+    if (!dpname) {
+        error = EINVAL;
+        dpctl_error(dpctl_p, error, "datapath not found");
+    } else {
+        error = parsed_dpif_open(dpname, false, dpif);
+        free(dpname);
+        if (error) {
+            dpctl_error(dpctl_p, error, "opening datapath");
+        }
+    }
+    return error;
+}
+
+static int
+dpctl_ct_set_maxconns(int argc, const char *argv[],
+                      struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
+    if (!error) {
+        uint32_t maxconns;
+        if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) {
+            error = ct_dpif_set_maxconns(dpif, maxconns);
+            dpif_close(dpif);
+
+            if (!error) {
+                dpctl_print(dpctl_p, "Ok");
+            } else {
+                dpctl_error(dpctl_p, error, "ct set maxconns failed");
+            }
+        } else {
+            error = EINVAL;
+            dpctl_error(dpctl_p, error, "maxconns missing or malformed");
+        }
+    }
+
+    return error;
+}
+
+static int
+dpctl_ct_get_maxconns(int argc, const char *argv[],
+                    struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
+    if (!error) {
+        uint32_t maxconns;
+        error = ct_dpif_get_maxconns(dpif, &maxconns);
+        dpif_close(dpif);
+
+        if (!error) {
+            dpctl_print(dpctl_p, "%u\n", maxconns);
+        } else {
+            dpctl_error(dpctl_p, error, "maxconns could not be retrieved");
+        }
+    }
+
+    return error;
+}
+
+static int
+dpctl_ct_get_nconns(int argc, const char *argv[],
+                    struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
+    if (!error) {
+        uint32_t nconns;
+        error = ct_dpif_get_nconns(dpif, &nconns);
+        dpif_close(dpif);
+
+        if (!error) {
+            dpctl_print(dpctl_p, "%u\n", nconns);
+        } else {
+            dpctl_error(dpctl_p, error, "nconns could not be retrieved");
+        }
+    }
+
+    return error;
+}
+
 /* Undocumented commands for unit testing. */
 
 static int
@@ -1950,6 +2040,9 @@ 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-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, DP_RW },
+    { "ct-get-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_get_maxconns, DP_RO },
+    { "ct-get-nconns", "[dp] nconns", 1, 2, dpctl_ct_get_nconns, 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 b1ef9a6..9432859 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5745,6 +5745,30 @@ dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone,
     return conntrack_flush(&dp->conntrack, zone);
 }
 
+static int
+dpif_netdev_ct_set_maxconns(struct dpif *dpif, uint32_t maxconns)
+{
+    struct dp_netdev *dp = get_dp_netdev(dpif);
+
+    return conntrack_set_maxconns(&dp->conntrack, maxconns);
+}
+
+static int
+dpif_netdev_ct_get_maxconns(struct dpif *dpif, uint32_t *maxconns)
+{
+    struct dp_netdev *dp = get_dp_netdev(dpif);
+
+    return conntrack_get_nconns(&dp->conntrack, maxconns);
+}
+
+static int
+dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns)
+{
+    struct dp_netdev *dp = get_dp_netdev(dpif);
+
+    return conntrack_get_nconns(&dp->conntrack, nconns);
+}
+
 const struct dpif_class dpif_netdev_class = {
     "netdev",
     dpif_netdev_init,
@@ -5790,6 +5814,9 @@ 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_maxconns,
+    dpif_netdev_ct_get_maxconns,
+    dpif_netdev_ct_get_nconns,
     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 3f76128..f8d75eb 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2989,6 +2989,9 @@ const struct dpif_class dpif_netlink_class = {
     dpif_netlink_ct_dump_next,
     dpif_netlink_ct_dump_done,
     dpif_netlink_ct_flush,
+    NULL,                       /* ct_set_maxconns */
+    NULL,                       /* ct_get_maxconns */
+    NULL,                       /* ct_get_nconns */
     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 947bf5e..62b3598 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -437,6 +437,12 @@ struct dpif_class {
      *     (zone 0). */
     int (*ct_flush)(struct dpif *, const uint16_t *zone,
                     const struct ct_dpif_tuple *tuple);
+    /* Set max connections allowed. */
+    int (*ct_set_maxconns)(struct dpif *, uint32_t maxconns);
+    /* Get max connections allowed. */
+    int (*ct_get_maxconns)(struct dpif *, uint32_t *maxconns);
+    /* Get number of connections tracked. */
+    int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);
 
     /* Meters */


///////////////////////////////////////////////////////////////



On 10/13/17, 1:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:

    Add infrastructure to implement:
     - dpctl/ct-get-glbl-cfg to read a current value of available
       conntrack parameters.
     - dpctl/ct-set-glbl-cfg to set a value to the available conntrack
       parameters.
    
    CC: Darrell Ball <dlu998@gmail.com>
    CC: Kevin Traynor <ktraynor@redhat.com>
    Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    ---
     lib/conntrack.c     | 60 ++++++++++++++++++++++++++++++++++++++++++
     lib/conntrack.h     |  3 +++
     lib/ct-dpif.c       | 28 ++++++++++++++++++++
     lib/ct-dpif.h       |  2 ++
     lib/dpctl.c         | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
     lib/dpif-netdev.c   | 19 ++++++++++++++
     lib/dpif-netlink.c  |  2 ++
     lib/dpif-provider.h |  4 +++
     8 files changed, 194 insertions(+)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index e555b55..29889c9 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_cfg_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);
    @@ -2485,6 +2492,59 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
         return 0;
     }
     
    +/* List of parameters that can be read/written at run-time. */
    +struct ct_cfg_params cfg_params[] = {};
    +
    +int
    +conntrack_set_param(struct conntrack *ct,
    +        const char *set_param)
    +{
    +    uint32_t max_conn;
    +    char buf[16] = "";
    +
    +    /* Check if the specified param can be managed. */
    +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
    +             i++) {
    +        if (!strncmp(set_param, cfg_params[i].cli,
    +                strlen(cfg_params[i].cli))) {
    +            ovs_strzcpy(buf, cfg_params[i].cli, sizeof(buf) - 1);
    +            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 - strlen(buf));
    +            if (ovs_scan(set_param, buf, &max_conn)) {
    +                return (cfg_params[i].wr
    +                        ? cfg_params[i].wr(ct, max_conn)
    +                        : EOPNOTSUPP);
    +            } else {
    +                return EINVAL;
    +            }
    +        }
    +    }
    +    /* The entered param didn't match any in the list. */
    +    VLOG_WARN("%s parameter is not managed by this command.", set_param);
    +
    +    return EINVAL;
    +}
    +
    +int
    +conntrack_get_param(struct conntrack *ct,
    +        const char *get_param, uint32_t *val)
    +{
    +    /* Check if the specified param can be managed. */
    +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
    +             i++) {
    +        if (!strncmp(get_param, cfg_params[i].cli,
    +                strlen(cfg_params[i].cli))) {
    +
    +            return (cfg_params[i].rd
    +                    ? cfg_params[i].rd(ct, val)
    +                    : EOPNOTSUPP);
    +        }
    +    }
    +    /* The entered param didn't match any in the list. */
    +    VLOG_WARN("%s parameter is not managed by this command.", get_param);
    +
    +    return EINVAL;
    +}
    +
     /* 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 b6eecf0..5b02951 100644
    --- a/lib/dpctl.c
    +++ b/lib/dpctl.c
    @@ -1589,6 +1589,80 @@ 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;
    +
    +    /* The datapath name is not a mandatory parameter for this command.
    +     * If it is not specified - so argc < 3 - we retrieve it from the
    +     * current setup, assuming only one exists. */
    +    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]);
    +
    +    if (!error) {
    +        dpctl_print(dpctl_p, "Ok");
    +    } else {
    +        dpctl_print(dpctl_p, "CT set global cfg failed (%s)",
    +                        ovs_strerror(error));
    +    }
    +
    +    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;
    +
    +    /* The datapath name is not a mandatory parameter for this command.
    +     * If it is not specified - so argc < 3 - we retrieve it from the
    +     * current setup, assuming only one exists. */
    +    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], &param_val);
    +
    +    if (!error) {
    +        dpctl_print(dpctl_p, "Current value: %d", param_val);
    +    } else {
    +        dpctl_print(dpctl_p, "CT get global cfg failed (%s)",
    +                        ovs_strerror(error));
    +    }
    +
    +    dpif_close(dpif);
    +
    +    return error;
    +}
    +
     ?
     /* Undocumented commands for unit testing. */
     
    @@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
    +    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644
    --- a/lib/dpif-netdev.c
    +++ b/lib/dpif-netdev.c
    @@ -5721,6 +5721,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,
    @@ -5766,6 +5783,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@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=m6PHM-SNZk7M88zlL6EQWY-S5kl3XTbB-YNfA-yk7xE&s=u0Gt2hVhSfd4NYQ0-mRy8r1P_U_vFyUqgE_Xd6rC9Qk&e=
Darrell Ball Dec. 11, 2017, 6:43 p.m. UTC | #2 
One extra note inline

Thanks Darrell

On 12/11/17, 8:35 AM, "Darrell Ball" <dball@vmware.com> wrote:

    Thanks Antonio for doing all this and pushing it forward.
    
    Regarding patches 2-4:
    
    I understand you want to save some code for various possible set and get operations.
    The prior art for these commands is however not generic set and get commands.
    Sometimes, we have specific commands that can take different numbers of arguments but
    those are specific and the context is clear.
    If we take some examples, we might see there is an issue with the approach here.
    Take per zone limits as one example, which is an internal requirement that we are working on
    across datapaths.
    This is a set operation with 2 arguments plus the datapath = 3 arguments.
    This won’t work with the generic set functions here.
    The corresponding get will not work either.
    Trying to make this work generically will get pretty messy and the parameter validation error prone.
    
    I would like to propose an alternative (a simple one; also with code consolidation and more error checking) below:
    Please let me know what you think.
    
    Thanks Darrell
    
    ///////////////////////////////////////////////////////////////
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index f5a3aa9..c8ad548 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -2485,6 +2485,27 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
         return 0;
     }
     
    +int
    +conntrack_set_maxconns(struct conntrack *ct, uint32_t maxconns)
    +{
    +    atomic_init(&ct->n_conn_limit, maxconns);
    +    return 0;
    +}
    +
    +int
    +conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns)
    +{
    +    atomic_read_relaxed(&ct->n_conn_limit, maxconns);
    +    return 0;
    +}
    +
    +int
    +conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns)
    +{
    +    *nconns = atomic_count_get(&ct->n_conn);
    +    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..8652724 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_maxconns(struct conntrack *ct, uint32_t maxconns);
    +int conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns);
    +int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns);
     ^L
     /* '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 239c848..5fa3a97 100644
    --- a/lib/ct-dpif.c
    +++ b/lib/ct-dpif.c
    @@ -140,6 +140,30 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone,
                 : EOPNOTSUPP);
     }
     
    +int
    +ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns)
    +{
    +    return (dpif->dpif_class->ct_set_maxconns
    +            ? dpif->dpif_class->ct_set_maxconns(dpif, maxconns)
    +            : EOPNOTSUPP);
    +}
    +
    +int
    +ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns)
    +{
    +    return (dpif->dpif_class->ct_get_maxconns
    +            ? dpif->dpif_class->ct_get_maxconns(dpif, maxconns)
    +            : EOPNOTSUPP);
    +}
    +
    +int
    +ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)
    +{
    +    return (dpif->dpif_class->ct_get_nconns
    +            ? dpif->dpif_class->ct_get_nconns(dpif, nconns)
    +            : EOPNOTSUPP);
    +}
    +
     void
     ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
     {
    diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
    index 5e2de53..09e7698 100644
    --- a/lib/ct-dpif.h
    +++ b/lib/ct-dpif.h
    @@ -197,6 +197,9 @@ 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,
                       const struct ct_dpif_tuple *);
    +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);
     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 a28ded9..215d1c3 100644
    --- a/lib/dpctl.c
    +++ b/lib/dpctl.c
    @@ -1654,6 +1654,96 @@ dpctl_ct_bkts(int argc, const char *argv[],
         return error;
     }
     ^L
    +static int
    +dpctl_ct_open_dp(int argc, const char *argv[],
    +                 struct dpctl_params *dpctl_p, struct dpif **dpif)
    +{
    +    int error = 0;
    +    /* The datapath name is not a mandatory parameter for this command.
    +     * If it is not specified - so argc < 3 - we retrieve it from the
    +     * current setup, assuming only one exists. */
    +    char *dpname = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);


[Darrell] The ‘3’ here would be replaced with a parameter representing the number
              of parameters expected when the dp is included in the parameter list.


    +    if (!dpname) {
    +        error = EINVAL;
    +        dpctl_error(dpctl_p, error, "datapath not found");
    +    } else {
    +        error = parsed_dpif_open(dpname, false, dpif);
    +        free(dpname);
    +        if (error) {
    +            dpctl_error(dpctl_p, error, "opening datapath");
    +        }
    +    }
    +    return error;
    +}
    +
    +static int
    +dpctl_ct_set_maxconns(int argc, const char *argv[],
    +                      struct dpctl_params *dpctl_p)
    +{
    +    struct dpif *dpif;
    +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
    +    if (!error) {
    +        uint32_t maxconns;
    +        if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) {
    +            error = ct_dpif_set_maxconns(dpif, maxconns);
    +            dpif_close(dpif);
    +
    +            if (!error) {
    +                dpctl_print(dpctl_p, "Ok");
    +            } else {
    +                dpctl_error(dpctl_p, error, "ct set maxconns failed");
    +            }
    +        } else {
    +            error = EINVAL;
    +            dpctl_error(dpctl_p, error, "maxconns missing or malformed");
    +        }
    +    }
    +
    +    return error;
    +}
    +
    +static int
    +dpctl_ct_get_maxconns(int argc, const char *argv[],
    +                    struct dpctl_params *dpctl_p)
    +{
    +    struct dpif *dpif;
    +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
    +    if (!error) {
    +        uint32_t maxconns;
    +        error = ct_dpif_get_maxconns(dpif, &maxconns);
    +        dpif_close(dpif);
    +
    +        if (!error) {
    +            dpctl_print(dpctl_p, "%u\n", maxconns);
    +        } else {
    +            dpctl_error(dpctl_p, error, "maxconns could not be retrieved");
    +        }
    +    }
    +
    +    return error;
    +}
    +
    +static int
    +dpctl_ct_get_nconns(int argc, const char *argv[],
    +                    struct dpctl_params *dpctl_p)
    +{
    +    struct dpif *dpif;
    +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);
    +    if (!error) {
    +        uint32_t nconns;
    +        error = ct_dpif_get_nconns(dpif, &nconns);
    +        dpif_close(dpif);
    +
    +        if (!error) {
    +            dpctl_print(dpctl_p, "%u\n", nconns);
    +        } else {
    +            dpctl_error(dpctl_p, error, "nconns could not be retrieved");
    +        }
    +    }
    +
    +    return error;
    +}
    +
     /* Undocumented commands for unit testing. */
     
     static int
    @@ -1950,6 +2040,9 @@ 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-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, DP_RW },
    +    { "ct-get-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_get_maxconns, DP_RO },
    +    { "ct-get-nconns", "[dp] nconns", 1, 2, dpctl_ct_get_nconns, 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 b1ef9a6..9432859 100644
    --- a/lib/dpif-netdev.c
    +++ b/lib/dpif-netdev.c
    @@ -5745,6 +5745,30 @@ dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone,
         return conntrack_flush(&dp->conntrack, zone);
     }
     
    +static int
    +dpif_netdev_ct_set_maxconns(struct dpif *dpif, uint32_t maxconns)
    +{
    +    struct dp_netdev *dp = get_dp_netdev(dpif);
    +
    +    return conntrack_set_maxconns(&dp->conntrack, maxconns);
    +}
    +
    +static int
    +dpif_netdev_ct_get_maxconns(struct dpif *dpif, uint32_t *maxconns)
    +{
    +    struct dp_netdev *dp = get_dp_netdev(dpif);
    +
    +    return conntrack_get_nconns(&dp->conntrack, maxconns);
    +}
    +
    +static int
    +dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns)
    +{
    +    struct dp_netdev *dp = get_dp_netdev(dpif);
    +
    +    return conntrack_get_nconns(&dp->conntrack, nconns);
    +}
    +
     const struct dpif_class dpif_netdev_class = {
         "netdev",
         dpif_netdev_init,
    @@ -5790,6 +5814,9 @@ 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_maxconns,
    +    dpif_netdev_ct_get_maxconns,
    +    dpif_netdev_ct_get_nconns,
         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 3f76128..f8d75eb 100644
    --- a/lib/dpif-netlink.c
    +++ b/lib/dpif-netlink.c
    @@ -2989,6 +2989,9 @@ const struct dpif_class dpif_netlink_class = {
         dpif_netlink_ct_dump_next,
         dpif_netlink_ct_dump_done,
         dpif_netlink_ct_flush,
    +    NULL,                       /* ct_set_maxconns */
    +    NULL,                       /* ct_get_maxconns */
    +    NULL,                       /* ct_get_nconns */
         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 947bf5e..62b3598 100644
    --- a/lib/dpif-provider.h
    +++ b/lib/dpif-provider.h
    @@ -437,6 +437,12 @@ struct dpif_class {
          *     (zone 0). */
         int (*ct_flush)(struct dpif *, const uint16_t *zone,
                         const struct ct_dpif_tuple *tuple);
    +    /* Set max connections allowed. */
    +    int (*ct_set_maxconns)(struct dpif *, uint32_t maxconns);
    +    /* Get max connections allowed. */
    +    int (*ct_get_maxconns)(struct dpif *, uint32_t *maxconns);
    +    /* Get number of connections tracked. */
    +    int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);
     
         /* Meters */
    
    
    ///////////////////////////////////////////////////////////////
    
    
    
    On 10/13/17, 1:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:
    
        Add infrastructure to implement:
         - dpctl/ct-get-glbl-cfg to read a current value of available
           conntrack parameters.
         - dpctl/ct-set-glbl-cfg to set a value to the available conntrack
           parameters.
        
        CC: Darrell Ball <dlu998@gmail.com>
        CC: Kevin Traynor <ktraynor@redhat.com>
        Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

        ---
         lib/conntrack.c     | 60 ++++++++++++++++++++++++++++++++++++++++++
         lib/conntrack.h     |  3 +++
         lib/ct-dpif.c       | 28 ++++++++++++++++++++
         lib/ct-dpif.h       |  2 ++
         lib/dpctl.c         | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
         lib/dpif-netdev.c   | 19 ++++++++++++++
         lib/dpif-netlink.c  |  2 ++
         lib/dpif-provider.h |  4 +++
         8 files changed, 194 insertions(+)
        
        diff --git a/lib/conntrack.c b/lib/conntrack.c
        index e555b55..29889c9 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_cfg_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);
        @@ -2485,6 +2492,59 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
             return 0;
         }
         
        +/* List of parameters that can be read/written at run-time. */
        +struct ct_cfg_params cfg_params[] = {};
        +
        +int
        +conntrack_set_param(struct conntrack *ct,
        +        const char *set_param)
        +{
        +    uint32_t max_conn;
        +    char buf[16] = "";
        +
        +    /* Check if the specified param can be managed. */
        +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
        +             i++) {
        +        if (!strncmp(set_param, cfg_params[i].cli,
        +                strlen(cfg_params[i].cli))) {
        +            ovs_strzcpy(buf, cfg_params[i].cli, sizeof(buf) - 1);
        +            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 - strlen(buf));
        +            if (ovs_scan(set_param, buf, &max_conn)) {
        +                return (cfg_params[i].wr
        +                        ? cfg_params[i].wr(ct, max_conn)
        +                        : EOPNOTSUPP);
        +            } else {
        +                return EINVAL;
        +            }
        +        }
        +    }
        +    /* The entered param didn't match any in the list. */
        +    VLOG_WARN("%s parameter is not managed by this command.", set_param);
        +
        +    return EINVAL;
        +}
        +
        +int
        +conntrack_get_param(struct conntrack *ct,
        +        const char *get_param, uint32_t *val)
        +{
        +    /* Check if the specified param can be managed. */
        +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
        +             i++) {
        +        if (!strncmp(get_param, cfg_params[i].cli,
        +                strlen(cfg_params[i].cli))) {
        +
        +            return (cfg_params[i].rd
        +                    ? cfg_params[i].rd(ct, val)
        +                    : EOPNOTSUPP);
        +        }
        +    }
        +    /* The entered param didn't match any in the list. */
        +    VLOG_WARN("%s parameter is not managed by this command.", get_param);
        +
        +    return EINVAL;
        +}
        +
         /* 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 b6eecf0..5b02951 100644
        --- a/lib/dpctl.c
        +++ b/lib/dpctl.c
        @@ -1589,6 +1589,80 @@ 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;
        +
        +    /* The datapath name is not a mandatory parameter for this command.
        +     * If it is not specified - so argc < 3 - we retrieve it from the
        +     * current setup, assuming only one exists. */
        +    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]);
        +
        +    if (!error) {
        +        dpctl_print(dpctl_p, "Ok");
        +    } else {
        +        dpctl_print(dpctl_p, "CT set global cfg failed (%s)",
        +                        ovs_strerror(error));
        +    }
        +
        +    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;
        +
        +    /* The datapath name is not a mandatory parameter for this command.
        +     * If it is not specified - so argc < 3 - we retrieve it from the
        +     * current setup, assuming only one exists. */
        +    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], &param_val);
        +
        +    if (!error) {
        +        dpctl_print(dpctl_p, "Current value: %d", param_val);
        +    } else {
        +        dpctl_print(dpctl_p, "CT get global cfg failed (%s)",
        +                        ovs_strerror(error));
        +    }
        +
        +    dpif_close(dpif);
        +
        +    return error;
        +}
        +
         ?
         /* Undocumented commands for unit testing. */
         
        @@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
        +    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644
        --- a/lib/dpif-netdev.c
        +++ b/lib/dpif-netdev.c
        @@ -5721,6 +5721,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,
        @@ -5766,6 +5783,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@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=m6PHM-SNZk7M88zlL6EQWY-S5kl3XTbB-YNfA-yk7xE&s=u0Gt2hVhSfd4NYQ0-mRy8r1P_U_vFyUqgE_Xd6rC9Qk&e=
Fischetti, Antonio Dec. 15, 2017, 7:05 p.m. UTC | #3 
Thanks Darrell for your review.
I agree with your comments, if we have to consider R/W commands with
more than one parameter is much better to have specific functions
as you explained and showed.

I'll rework accordingly and post a v4.

Regards,
-Antonio

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com]

> Sent: Monday, December 11, 2017 6:44 PM

> To: Fischetti, Antonio <antonio.fischetti@intel.com>;

> dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH v3 2/5] conntrack: add commands to r/w CT

> parameters.

> 

> One extra note inline

> 

> Thanks Darrell

> 

> On 12/11/17, 8:35 AM, "Darrell Ball" <dball@vmware.com> wrote:

> 

>     Thanks Antonio for doing all this and pushing it forward.

> 

>     Regarding patches 2-4:

> 

>     I understand you want to save some code for various possible set and

> get operations.

>     The prior art for these commands is however not generic set and get

> commands.

>     Sometimes, we have specific commands that can take different numbers

> of arguments but

>     those are specific and the context is clear.

>     If we take some examples, we might see there is an issue with the

> approach here.

>     Take per zone limits as one example, which is an internal

> requirement that we are working on

>     across datapaths.

>     This is a set operation with 2 arguments plus the datapath = 3

> arguments.

>     This won’t work with the generic set functions here.

>     The corresponding get will not work either.

>     Trying to make this work generically will get pretty messy and the

> parameter validation error prone.

> 

>     I would like to propose an alternative (a simple one; also with code

> consolidation and more error checking) below:

>     Please let me know what you think.

> 

>     Thanks Darrell

> 

>     ///////////////////////////////////////////////////////////////

> 

>     diff --git a/lib/conntrack.c b/lib/conntrack.c

>     index f5a3aa9..c8ad548 100644

>     --- a/lib/conntrack.c

>     +++ b/lib/conntrack.c

>     @@ -2485,6 +2485,27 @@ conntrack_flush(struct conntrack *ct, const

> uint16_t *zone)

>          return 0;

>      }

> 

>     +int

>     +conntrack_set_maxconns(struct conntrack *ct, uint32_t maxconns)

>     +{

>     +    atomic_init(&ct->n_conn_limit, maxconns);

>     +    return 0;

>     +}

>     +

>     +int

>     +conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns)

>     +{

>     +    atomic_read_relaxed(&ct->n_conn_limit, maxconns);

>     +    return 0;

>     +}

>     +

>     +int

>     +conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns)

>     +{

>     +    *nconns = atomic_count_get(&ct->n_conn);

>     +    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..8652724 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_maxconns(struct conntrack *ct, uint32_t

> maxconns);

>     +int conntrack_get_maxconns(struct conntrack *ct, uint32_t

> *maxconns);

>     +int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns);

>      ^L

>      /* '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 239c848..5fa3a97 100644

>     --- a/lib/ct-dpif.c

>     +++ b/lib/ct-dpif.c

>     @@ -140,6 +140,30 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t

> *zone,

>                  : EOPNOTSUPP);

>      }

> 

>     +int

>     +ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns)

>     +{

>     +    return (dpif->dpif_class->ct_set_maxconns

>     +            ? dpif->dpif_class->ct_set_maxconns(dpif, maxconns)

>     +            : EOPNOTSUPP);

>     +}

>     +

>     +int

>     +ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns)

>     +{

>     +    return (dpif->dpif_class->ct_get_maxconns

>     +            ? dpif->dpif_class->ct_get_maxconns(dpif, maxconns)

>     +            : EOPNOTSUPP);

>     +}

>     +

>     +int

>     +ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)

>     +{

>     +    return (dpif->dpif_class->ct_get_nconns

>     +            ? dpif->dpif_class->ct_get_nconns(dpif, nconns)

>     +            : EOPNOTSUPP);

>     +}

>     +

>      void

>      ct_dpif_entry_uninit(struct ct_dpif_entry *entry)

>      {

>     diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h

>     index 5e2de53..09e7698 100644

>     --- a/lib/ct-dpif.h

>     +++ b/lib/ct-dpif.h

>     @@ -197,6 +197,9 @@ 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,

>                        const struct ct_dpif_tuple *);

>     +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);

>      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 a28ded9..215d1c3 100644

>     --- a/lib/dpctl.c

>     +++ b/lib/dpctl.c

>     @@ -1654,6 +1654,96 @@ dpctl_ct_bkts(int argc, const char *argv[],

>          return error;

>      }

>      ^L

>     +static int

>     +dpctl_ct_open_dp(int argc, const char *argv[],

>     +                 struct dpctl_params *dpctl_p, struct dpif **dpif)

>     +{

>     +    int error = 0;

>     +    /* The datapath name is not a mandatory parameter for this

> command.

>     +     * If it is not specified - so argc < 3 - we retrieve it from

> the

>     +     * current setup, assuming only one exists. */

>     +    char *dpname = argc == 3 ? xstrdup(argv[1]) :

> get_one_dp(dpctl_p);

> 

> 

> [Darrell] The ‘3’ here would be replaced with a parameter representing

> the number

>               of parameters expected when the dp is included in the

> parameter list.

> 

> 

>     +    if (!dpname) {

>     +        error = EINVAL;

>     +        dpctl_error(dpctl_p, error, "datapath not found");

>     +    } else {

>     +        error = parsed_dpif_open(dpname, false, dpif);

>     +        free(dpname);

>     +        if (error) {

>     +            dpctl_error(dpctl_p, error, "opening datapath");

>     +        }

>     +    }

>     +    return error;

>     +}

>     +

>     +static int

>     +dpctl_ct_set_maxconns(int argc, const char *argv[],

>     +                      struct dpctl_params *dpctl_p)

>     +{

>     +    struct dpif *dpif;

>     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     +    if (!error) {

>     +        uint32_t maxconns;

>     +        if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) {

>     +            error = ct_dpif_set_maxconns(dpif, maxconns);

>     +            dpif_close(dpif);

>     +

>     +            if (!error) {

>     +                dpctl_print(dpctl_p, "Ok");

>     +            } else {

>     +                dpctl_error(dpctl_p, error, "ct set maxconns

> failed");

>     +            }

>     +        } else {

>     +            error = EINVAL;

>     +            dpctl_error(dpctl_p, error, "maxconns missing or

> malformed");

>     +        }

>     +    }

>     +

>     +    return error;

>     +}

>     +

>     +static int

>     +dpctl_ct_get_maxconns(int argc, const char *argv[],

>     +                    struct dpctl_params *dpctl_p)

>     +{

>     +    struct dpif *dpif;

>     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     +    if (!error) {

>     +        uint32_t maxconns;

>     +        error = ct_dpif_get_maxconns(dpif, &maxconns);

>     +        dpif_close(dpif);

>     +

>     +        if (!error) {

>     +            dpctl_print(dpctl_p, "%u\n", maxconns);

>     +        } else {

>     +            dpctl_error(dpctl_p, error, "maxconns could not be

> retrieved");

>     +        }

>     +    }

>     +

>     +    return error;

>     +}

>     +

>     +static int

>     +dpctl_ct_get_nconns(int argc, const char *argv[],

>     +                    struct dpctl_params *dpctl_p)

>     +{

>     +    struct dpif *dpif;

>     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     +    if (!error) {

>     +        uint32_t nconns;

>     +        error = ct_dpif_get_nconns(dpif, &nconns);

>     +        dpif_close(dpif);

>     +

>     +        if (!error) {

>     +            dpctl_print(dpctl_p, "%u\n", nconns);

>     +        } else {

>     +            dpctl_error(dpctl_p, error, "nconns could not be

> retrieved");

>     +        }

>     +    }

>     +

>     +    return error;

>     +}

>     +

>      /* Undocumented commands for unit testing. */

> 

>      static int

>     @@ -1950,6 +2040,9 @@ 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-maxconns", "[dp] maxconns", 1, 2,

> dpctl_ct_set_maxconns, DP_RW },

>     +    { "ct-get-maxconns", "[dp] maxconns", 1, 2,

> dpctl_ct_get_maxconns, DP_RO },

>     +    { "ct-get-nconns", "[dp] nconns", 1, 2, dpctl_ct_get_nconns,

> 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 b1ef9a6..9432859 100644

>     --- a/lib/dpif-netdev.c

>     +++ b/lib/dpif-netdev.c

>     @@ -5745,6 +5745,30 @@ dpif_netdev_ct_flush(struct dpif *dpif, const

> uint16_t *zone,

>          return conntrack_flush(&dp->conntrack, zone);

>      }

> 

>     +static int

>     +dpif_netdev_ct_set_maxconns(struct dpif *dpif, uint32_t maxconns)

>     +{

>     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     +

>     +    return conntrack_set_maxconns(&dp->conntrack, maxconns);

>     +}

>     +

>     +static int

>     +dpif_netdev_ct_get_maxconns(struct dpif *dpif, uint32_t *maxconns)

>     +{

>     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     +

>     +    return conntrack_get_nconns(&dp->conntrack, maxconns);

>     +}

>     +

>     +static int

>     +dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns)

>     +{

>     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     +

>     +    return conntrack_get_nconns(&dp->conntrack, nconns);

>     +}

>     +

>      const struct dpif_class dpif_netdev_class = {

>          "netdev",

>          dpif_netdev_init,

>     @@ -5790,6 +5814,9 @@ 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_maxconns,

>     +    dpif_netdev_ct_get_maxconns,

>     +    dpif_netdev_ct_get_nconns,

>          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 3f76128..f8d75eb 100644

>     --- a/lib/dpif-netlink.c

>     +++ b/lib/dpif-netlink.c

>     @@ -2989,6 +2989,9 @@ const struct dpif_class dpif_netlink_class = {

>          dpif_netlink_ct_dump_next,

>          dpif_netlink_ct_dump_done,

>          dpif_netlink_ct_flush,

>     +    NULL,                       /* ct_set_maxconns */

>     +    NULL,                       /* ct_get_maxconns */

>     +    NULL,                       /* ct_get_nconns */

>          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 947bf5e..62b3598 100644

>     --- a/lib/dpif-provider.h

>     +++ b/lib/dpif-provider.h

>     @@ -437,6 +437,12 @@ struct dpif_class {

>           *     (zone 0). */

>          int (*ct_flush)(struct dpif *, const uint16_t *zone,

>                          const struct ct_dpif_tuple *tuple);

>     +    /* Set max connections allowed. */

>     +    int (*ct_set_maxconns)(struct dpif *, uint32_t maxconns);

>     +    /* Get max connections allowed. */

>     +    int (*ct_get_maxconns)(struct dpif *, uint32_t *maxconns);

>     +    /* Get number of connections tracked. */

>     +    int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);

> 

>          /* Meters */

> 

> 

>     ///////////////////////////////////////////////////////////////

> 

> 

> 

>     On 10/13/17, 1:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of

> antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf

> of antonio.fischetti@intel.com> wrote:

> 

>         Add infrastructure to implement:

>          - dpctl/ct-get-glbl-cfg to read a current value of available

>            conntrack parameters.

>          - dpctl/ct-set-glbl-cfg to set a value to the available

> conntrack

>            parameters.

> 

>         CC: Darrell Ball <dlu998@gmail.com>

>         CC: Kevin Traynor <ktraynor@redhat.com>

>         Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

>         ---

>          lib/conntrack.c     | 60

> ++++++++++++++++++++++++++++++++++++++++++

>          lib/conntrack.h     |  3 +++

>          lib/ct-dpif.c       | 28 ++++++++++++++++++++

>          lib/ct-dpif.h       |  2 ++

>          lib/dpctl.c         | 76

> +++++++++++++++++++++++++++++++++++++++++++++++++++++

>          lib/dpif-netdev.c   | 19 ++++++++++++++

>          lib/dpif-netlink.c  |  2 ++

>          lib/dpif-provider.h |  4 +++

>          8 files changed, 194 insertions(+)

> 

>         diff --git a/lib/conntrack.c b/lib/conntrack.c

>         index e555b55..29889c9 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_cfg_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);

>         @@ -2485,6 +2492,59 @@ conntrack_flush(struct conntrack *ct,

> const uint16_t *zone)

>              return 0;

>          }

> 

>         +/* List of parameters that can be read/written at run-time. */

>         +struct ct_cfg_params cfg_params[] = {};

>         +

>         +int

>         +conntrack_set_param(struct conntrack *ct,

>         +        const char *set_param)

>         +{

>         +    uint32_t max_conn;

>         +    char buf[16] = "";

>         +

>         +    /* Check if the specified param can be managed. */

>         +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct

> ct_cfg_params);

>         +             i++) {

>         +        if (!strncmp(set_param, cfg_params[i].cli,

>         +                strlen(cfg_params[i].cli))) {

>         +            ovs_strzcpy(buf, cfg_params[i].cli, sizeof(buf) -

> 1);

>         +            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 -

> strlen(buf));

>         +            if (ovs_scan(set_param, buf, &max_conn)) {

>         +                return (cfg_params[i].wr

>         +                        ? cfg_params[i].wr(ct, max_conn)

>         +                        : EOPNOTSUPP);

>         +            } else {

>         +                return EINVAL;

>         +            }

>         +        }

>         +    }

>         +    /* The entered param didn't match any in the list. */

>         +    VLOG_WARN("%s parameter is not managed by this command.",

> set_param);

>         +

>         +    return EINVAL;

>         +}

>         +

>         +int

>         +conntrack_get_param(struct conntrack *ct,

>         +        const char *get_param, uint32_t *val)

>         +{

>         +    /* Check if the specified param can be managed. */

>         +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct

> ct_cfg_params);

>         +             i++) {

>         +        if (!strncmp(get_param, cfg_params[i].cli,

>         +                strlen(cfg_params[i].cli))) {

>         +

>         +            return (cfg_params[i].rd

>         +                    ? cfg_params[i].rd(ct, val)

>         +                    : EOPNOTSUPP);

>         +        }

>         +    }

>         +    /* The entered param didn't match any in the list. */

>         +    VLOG_WARN("%s parameter is not managed by this command.",

> get_param);

>         +

>         +    return EINVAL;

>         +}

>         +

>          /* 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 b6eecf0..5b02951 100644

>         --- a/lib/dpctl.c

>         +++ b/lib/dpctl.c

>         @@ -1589,6 +1589,80 @@ 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;

>         +

>         +    /* The datapath name is not a mandatory parameter for this

> command.

>         +     * If it is not specified - so argc < 3 - we retrieve it

> from the

>         +     * current setup, assuming only one exists. */

>         +    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]);

>         +

>         +    if (!error) {

>         +        dpctl_print(dpctl_p, "Ok");

>         +    } else {

>         +        dpctl_print(dpctl_p, "CT set global cfg failed (%s)",

>         +                        ovs_strerror(error));

>         +    }

>         +

>         +    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;

>         +

>         +    /* The datapath name is not a mandatory parameter for this

> command.

>         +     * If it is not specified - so argc < 3 - we retrieve it

> from the

>         +     * current setup, assuming only one exists. */

>         +    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],

> &param_val);

>         +

>         +    if (!error) {

>         +        dpctl_print(dpctl_p, "Current value: %d", param_val);

>         +    } else {

>         +        dpctl_print(dpctl_p, "CT get global cfg failed (%s)",

>         +                        ovs_strerror(error));

>         +    }

>         +

>         +    dpif_close(dpif);

>         +

>         +    return error;

>         +}

>         +

>          ?

>          /* Undocumented commands for unit testing. */

> 

>         @@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2,

> dpctl_ct_set_param, DP_RW },

>         +    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644

>         --- a/lib/dpif-netdev.c

>         +++ b/lib/dpif-netdev.c

>         @@ -5721,6 +5721,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,

>         @@ -5766,6 +5783,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@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=m6PHM-SNZk7M88zlL6EQWY-S5kl3XTbB-YNfA-yk7xE&s=u0Gt2hVhSfd4NYQ0-

> mRy8r1P_U_vFyUqgE_Xd6rC9Qk&e=

> 

> 

> 

> 

> 

> 

> 

> 

>
Darrell Ball Dec. 15, 2017, 7:23 p.m. UTC | #4 
Hi Antonio

Alternatively, given that most of code is rewritten, I was planning on just submitting the patches, which are in process, and adding you as co-author.
Will that work for you?

Thanks Darrell



On 12/15/17, 11:05 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote:

    Thanks Darrell for your review.
    I agree with your comments, if we have to consider R/W commands with
    more than one parameter is much better to have specific functions
    as you explained and showed.
    
    I'll rework accordingly and post a v4.
    
    Regards,
    -Antonio
    
    > -----Original Message-----

    > From: Darrell Ball [mailto:dball@vmware.com]

    > Sent: Monday, December 11, 2017 6:44 PM

    > To: Fischetti, Antonio <antonio.fischetti@intel.com>;

    > dev@openvswitch.org

    > Subject: Re: [ovs-dev] [PATCH v3 2/5] conntrack: add commands to r/w CT

    > parameters.

    > 

    > One extra note inline

    > 

    > Thanks Darrell

    > 

    > On 12/11/17, 8:35 AM, "Darrell Ball" <dball@vmware.com> wrote:

    > 

    >     Thanks Antonio for doing all this and pushing it forward.

    > 

    >     Regarding patches 2-4:

    > 

    >     I understand you want to save some code for various possible set and

    > get operations.

    >     The prior art for these commands is however not generic set and get

    > commands.

    >     Sometimes, we have specific commands that can take different numbers

    > of arguments but

    >     those are specific and the context is clear.

    >     If we take some examples, we might see there is an issue with the

    > approach here.

    >     Take per zone limits as one example, which is an internal

    > requirement that we are working on

    >     across datapaths.

    >     This is a set operation with 2 arguments plus the datapath = 3

    > arguments.

    >     This won’t work with the generic set functions here.

    >     The corresponding get will not work either.

    >     Trying to make this work generically will get pretty messy and the

    > parameter validation error prone.

    > 

    >     I would like to propose an alternative (a simple one; also with code

    > consolidation and more error checking) below:

    >     Please let me know what you think.

    > 

    >     Thanks Darrell

    > 

    >     ///////////////////////////////////////////////////////////////

    > 

    >     diff --git a/lib/conntrack.c b/lib/conntrack.c

    >     index f5a3aa9..c8ad548 100644

    >     --- a/lib/conntrack.c

    >     +++ b/lib/conntrack.c

    >     @@ -2485,6 +2485,27 @@ conntrack_flush(struct conntrack *ct, const

    > uint16_t *zone)

    >          return 0;

    >      }

    > 

    >     +int

    >     +conntrack_set_maxconns(struct conntrack *ct, uint32_t maxconns)

    >     +{

    >     +    atomic_init(&ct->n_conn_limit, maxconns);

    >     +    return 0;

    >     +}

    >     +

    >     +int

    >     +conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns)

    >     +{

    >     +    atomic_read_relaxed(&ct->n_conn_limit, maxconns);

    >     +    return 0;

    >     +}

    >     +

    >     +int

    >     +conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns)

    >     +{

    >     +    *nconns = atomic_count_get(&ct->n_conn);

    >     +    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..8652724 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_maxconns(struct conntrack *ct, uint32_t

    > maxconns);

    >     +int conntrack_get_maxconns(struct conntrack *ct, uint32_t

    > *maxconns);

    >     +int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns);

    >      ^L

    >      /* '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 239c848..5fa3a97 100644

    >     --- a/lib/ct-dpif.c

    >     +++ b/lib/ct-dpif.c

    >     @@ -140,6 +140,30 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t

    > *zone,

    >                  : EOPNOTSUPP);

    >      }

    > 

    >     +int

    >     +ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns)

    >     +{

    >     +    return (dpif->dpif_class->ct_set_maxconns

    >     +            ? dpif->dpif_class->ct_set_maxconns(dpif, maxconns)

    >     +            : EOPNOTSUPP);

    >     +}

    >     +

    >     +int

    >     +ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns)

    >     +{

    >     +    return (dpif->dpif_class->ct_get_maxconns

    >     +            ? dpif->dpif_class->ct_get_maxconns(dpif, maxconns)

    >     +            : EOPNOTSUPP);

    >     +}

    >     +

    >     +int

    >     +ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)

    >     +{

    >     +    return (dpif->dpif_class->ct_get_nconns

    >     +            ? dpif->dpif_class->ct_get_nconns(dpif, nconns)

    >     +            : EOPNOTSUPP);

    >     +}

    >     +

    >      void

    >      ct_dpif_entry_uninit(struct ct_dpif_entry *entry)

    >      {

    >     diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h

    >     index 5e2de53..09e7698 100644

    >     --- a/lib/ct-dpif.h

    >     +++ b/lib/ct-dpif.h

    >     @@ -197,6 +197,9 @@ 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,

    >                        const struct ct_dpif_tuple *);

    >     +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);

    >      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 a28ded9..215d1c3 100644

    >     --- a/lib/dpctl.c

    >     +++ b/lib/dpctl.c

    >     @@ -1654,6 +1654,96 @@ dpctl_ct_bkts(int argc, const char *argv[],

    >          return error;

    >      }

    >      ^L

    >     +static int

    >     +dpctl_ct_open_dp(int argc, const char *argv[],

    >     +                 struct dpctl_params *dpctl_p, struct dpif **dpif)

    >     +{

    >     +    int error = 0;

    >     +    /* The datapath name is not a mandatory parameter for this

    > command.

    >     +     * If it is not specified - so argc < 3 - we retrieve it from

    > the

    >     +     * current setup, assuming only one exists. */

    >     +    char *dpname = argc == 3 ? xstrdup(argv[1]) :

    > get_one_dp(dpctl_p);

    > 

    > 

    > [Darrell] The ‘3’ here would be replaced with a parameter representing

    > the number

    >               of parameters expected when the dp is included in the

    > parameter list.

    > 

    > 

    >     +    if (!dpname) {

    >     +        error = EINVAL;

    >     +        dpctl_error(dpctl_p, error, "datapath not found");

    >     +    } else {

    >     +        error = parsed_dpif_open(dpname, false, dpif);

    >     +        free(dpname);

    >     +        if (error) {

    >     +            dpctl_error(dpctl_p, error, "opening datapath");

    >     +        }

    >     +    }

    >     +    return error;

    >     +}

    >     +

    >     +static int

    >     +dpctl_ct_set_maxconns(int argc, const char *argv[],

    >     +                      struct dpctl_params *dpctl_p)

    >     +{

    >     +    struct dpif *dpif;

    >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

    >     +    if (!error) {

    >     +        uint32_t maxconns;

    >     +        if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) {

    >     +            error = ct_dpif_set_maxconns(dpif, maxconns);

    >     +            dpif_close(dpif);

    >     +

    >     +            if (!error) {

    >     +                dpctl_print(dpctl_p, "Ok");

    >     +            } else {

    >     +                dpctl_error(dpctl_p, error, "ct set maxconns

    > failed");

    >     +            }

    >     +        } else {

    >     +            error = EINVAL;

    >     +            dpctl_error(dpctl_p, error, "maxconns missing or

    > malformed");

    >     +        }

    >     +    }

    >     +

    >     +    return error;

    >     +}

    >     +

    >     +static int

    >     +dpctl_ct_get_maxconns(int argc, const char *argv[],

    >     +                    struct dpctl_params *dpctl_p)

    >     +{

    >     +    struct dpif *dpif;

    >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

    >     +    if (!error) {

    >     +        uint32_t maxconns;

    >     +        error = ct_dpif_get_maxconns(dpif, &maxconns);

    >     +        dpif_close(dpif);

    >     +

    >     +        if (!error) {

    >     +            dpctl_print(dpctl_p, "%u\n", maxconns);

    >     +        } else {

    >     +            dpctl_error(dpctl_p, error, "maxconns could not be

    > retrieved");

    >     +        }

    >     +    }

    >     +

    >     +    return error;

    >     +}

    >     +

    >     +static int

    >     +dpctl_ct_get_nconns(int argc, const char *argv[],

    >     +                    struct dpctl_params *dpctl_p)

    >     +{

    >     +    struct dpif *dpif;

    >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

    >     +    if (!error) {

    >     +        uint32_t nconns;

    >     +        error = ct_dpif_get_nconns(dpif, &nconns);

    >     +        dpif_close(dpif);

    >     +

    >     +        if (!error) {

    >     +            dpctl_print(dpctl_p, "%u\n", nconns);

    >     +        } else {

    >     +            dpctl_error(dpctl_p, error, "nconns could not be

    > retrieved");

    >     +        }

    >     +    }

    >     +

    >     +    return error;

    >     +}

    >     +

    >      /* Undocumented commands for unit testing. */

    > 

    >      static int

    >     @@ -1950,6 +2040,9 @@ 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-maxconns", "[dp] maxconns", 1, 2,

    > dpctl_ct_set_maxconns, DP_RW },

    >     +    { "ct-get-maxconns", "[dp] maxconns", 1, 2,

    > dpctl_ct_get_maxconns, DP_RO },

    >     +    { "ct-get-nconns", "[dp] nconns", 1, 2, dpctl_ct_get_nconns,

    > 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 b1ef9a6..9432859 100644

    >     --- a/lib/dpif-netdev.c

    >     +++ b/lib/dpif-netdev.c

    >     @@ -5745,6 +5745,30 @@ dpif_netdev_ct_flush(struct dpif *dpif, const

    > uint16_t *zone,

    >          return conntrack_flush(&dp->conntrack, zone);

    >      }

    > 

    >     +static int

    >     +dpif_netdev_ct_set_maxconns(struct dpif *dpif, uint32_t maxconns)

    >     +{

    >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

    >     +

    >     +    return conntrack_set_maxconns(&dp->conntrack, maxconns);

    >     +}

    >     +

    >     +static int

    >     +dpif_netdev_ct_get_maxconns(struct dpif *dpif, uint32_t *maxconns)

    >     +{

    >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

    >     +

    >     +    return conntrack_get_nconns(&dp->conntrack, maxconns);

    >     +}

    >     +

    >     +static int

    >     +dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t *nconns)

    >     +{

    >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

    >     +

    >     +    return conntrack_get_nconns(&dp->conntrack, nconns);

    >     +}

    >     +

    >      const struct dpif_class dpif_netdev_class = {

    >          "netdev",

    >          dpif_netdev_init,

    >     @@ -5790,6 +5814,9 @@ 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_maxconns,

    >     +    dpif_netdev_ct_get_maxconns,

    >     +    dpif_netdev_ct_get_nconns,

    >          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 3f76128..f8d75eb 100644

    >     --- a/lib/dpif-netlink.c

    >     +++ b/lib/dpif-netlink.c

    >     @@ -2989,6 +2989,9 @@ const struct dpif_class dpif_netlink_class = {

    >          dpif_netlink_ct_dump_next,

    >          dpif_netlink_ct_dump_done,

    >          dpif_netlink_ct_flush,

    >     +    NULL,                       /* ct_set_maxconns */

    >     +    NULL,                       /* ct_get_maxconns */

    >     +    NULL,                       /* ct_get_nconns */

    >          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 947bf5e..62b3598 100644

    >     --- a/lib/dpif-provider.h

    >     +++ b/lib/dpif-provider.h

    >     @@ -437,6 +437,12 @@ struct dpif_class {

    >           *     (zone 0). */

    >          int (*ct_flush)(struct dpif *, const uint16_t *zone,

    >                          const struct ct_dpif_tuple *tuple);

    >     +    /* Set max connections allowed. */

    >     +    int (*ct_set_maxconns)(struct dpif *, uint32_t maxconns);

    >     +    /* Get max connections allowed. */

    >     +    int (*ct_get_maxconns)(struct dpif *, uint32_t *maxconns);

    >     +    /* Get number of connections tracked. */

    >     +    int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);

    > 

    >          /* Meters */

    > 

    > 

    >     ///////////////////////////////////////////////////////////////

    > 

    > 

    > 

    >     On 10/13/17, 1:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of

    > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf

    > of antonio.fischetti@intel.com> wrote:

    > 

    >         Add infrastructure to implement:

    >          - dpctl/ct-get-glbl-cfg to read a current value of available

    >            conntrack parameters.

    >          - dpctl/ct-set-glbl-cfg to set a value to the available

    > conntrack

    >            parameters.

    > 

    >         CC: Darrell Ball <dlu998@gmail.com>

    >         CC: Kevin Traynor <ktraynor@redhat.com>

    >         Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    >         ---

    >          lib/conntrack.c     | 60

    > ++++++++++++++++++++++++++++++++++++++++++

    >          lib/conntrack.h     |  3 +++

    >          lib/ct-dpif.c       | 28 ++++++++++++++++++++

    >          lib/ct-dpif.h       |  2 ++

    >          lib/dpctl.c         | 76

    > +++++++++++++++++++++++++++++++++++++++++++++++++++++

    >          lib/dpif-netdev.c   | 19 ++++++++++++++

    >          lib/dpif-netlink.c  |  2 ++

    >          lib/dpif-provider.h |  4 +++

    >          8 files changed, 194 insertions(+)

    > 

    >         diff --git a/lib/conntrack.c b/lib/conntrack.c

    >         index e555b55..29889c9 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_cfg_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);

    >         @@ -2485,6 +2492,59 @@ conntrack_flush(struct conntrack *ct,

    > const uint16_t *zone)

    >              return 0;

    >          }

    > 

    >         +/* List of parameters that can be read/written at run-time. */

    >         +struct ct_cfg_params cfg_params[] = {};

    >         +

    >         +int

    >         +conntrack_set_param(struct conntrack *ct,

    >         +        const char *set_param)

    >         +{

    >         +    uint32_t max_conn;

    >         +    char buf[16] = "";

    >         +

    >         +    /* Check if the specified param can be managed. */

    >         +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct

    > ct_cfg_params);

    >         +             i++) {

    >         +        if (!strncmp(set_param, cfg_params[i].cli,

    >         +                strlen(cfg_params[i].cli))) {

    >         +            ovs_strzcpy(buf, cfg_params[i].cli, sizeof(buf) -

    > 1);

    >         +            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 -

    > strlen(buf));

    >         +            if (ovs_scan(set_param, buf, &max_conn)) {

    >         +                return (cfg_params[i].wr

    >         +                        ? cfg_params[i].wr(ct, max_conn)

    >         +                        : EOPNOTSUPP);

    >         +            } else {

    >         +                return EINVAL;

    >         +            }

    >         +        }

    >         +    }

    >         +    /* The entered param didn't match any in the list. */

    >         +    VLOG_WARN("%s parameter is not managed by this command.",

    > set_param);

    >         +

    >         +    return EINVAL;

    >         +}

    >         +

    >         +int

    >         +conntrack_get_param(struct conntrack *ct,

    >         +        const char *get_param, uint32_t *val)

    >         +{

    >         +    /* Check if the specified param can be managed. */

    >         +    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct

    > ct_cfg_params);

    >         +             i++) {

    >         +        if (!strncmp(get_param, cfg_params[i].cli,

    >         +                strlen(cfg_params[i].cli))) {

    >         +

    >         +            return (cfg_params[i].rd

    >         +                    ? cfg_params[i].rd(ct, val)

    >         +                    : EOPNOTSUPP);

    >         +        }

    >         +    }

    >         +    /* The entered param didn't match any in the list. */

    >         +    VLOG_WARN("%s parameter is not managed by this command.",

    > get_param);

    >         +

    >         +    return EINVAL;

    >         +}

    >         +

    >          /* 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 b6eecf0..5b02951 100644

    >         --- a/lib/dpctl.c

    >         +++ b/lib/dpctl.c

    >         @@ -1589,6 +1589,80 @@ 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;

    >         +

    >         +    /* The datapath name is not a mandatory parameter for this

    > command.

    >         +     * If it is not specified - so argc < 3 - we retrieve it

    > from the

    >         +     * current setup, assuming only one exists. */

    >         +    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]);

    >         +

    >         +    if (!error) {

    >         +        dpctl_print(dpctl_p, "Ok");

    >         +    } else {

    >         +        dpctl_print(dpctl_p, "CT set global cfg failed (%s)",

    >         +                        ovs_strerror(error));

    >         +    }

    >         +

    >         +    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;

    >         +

    >         +    /* The datapath name is not a mandatory parameter for this

    > command.

    >         +     * If it is not specified - so argc < 3 - we retrieve it

    > from the

    >         +     * current setup, assuming only one exists. */

    >         +    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],

    > &param_val);

    >         +

    >         +    if (!error) {

    >         +        dpctl_print(dpctl_p, "Current value: %d", param_val);

    >         +    } else {

    >         +        dpctl_print(dpctl_p, "CT get global cfg failed (%s)",

    >         +                        ovs_strerror(error));

    >         +    }

    >         +

    >         +    dpif_close(dpif);

    >         +

    >         +    return error;

    >         +}

    >         +

    >          ?

    >          /* Undocumented commands for unit testing. */

    > 

    >         @@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2,

    > dpctl_ct_set_param, DP_RW },

    >         +    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644

    >         --- a/lib/dpif-netdev.c

    >         +++ b/lib/dpif-netdev.c

    >         @@ -5721,6 +5721,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,

    >         @@ -5766,6 +5783,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@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=m6PHM-SNZk7M88zlL6EQWY-S5kl3XTbB-YNfA-yk7xE&s=u0Gt2hVhSfd4NYQ0-

    > mRy8r1P_U_vFyUqgE_Xd6rC9Qk&e=

    > 

    > 

    > 

    > 

    > 

    > 

    > 

    > 

    >
Fischetti, Antonio Dec. 15, 2017, 7:44 p.m. UTC | #5 
Sure, that's perfect.

Thanks, Antonio

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com]

> Sent: Friday, December 15, 2017 7:23 PM

> To: Fischetti, Antonio <antonio.fischetti@intel.com>;

> dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH v3 2/5] conntrack: add commands to r/w CT

> parameters.

> 

> Hi Antonio

> 

> Alternatively, given that most of code is rewritten, I was planning on

> just submitting the patches, which are in process, and adding you as co-

> author.

> Will that work for you?

> 

> Thanks Darrell

> 

> 

> 

> On 12/15/17, 11:05 AM, "Fischetti, Antonio"

> <antonio.fischetti@intel.com> wrote:

> 

>     Thanks Darrell for your review.

>     I agree with your comments, if we have to consider R/W commands with

>     more than one parameter is much better to have specific functions

>     as you explained and showed.

> 

>     I'll rework accordingly and post a v4.

> 

>     Regards,

>     -Antonio

> 

>     > -----Original Message-----

>     > From: Darrell Ball [mailto:dball@vmware.com]

>     > Sent: Monday, December 11, 2017 6:44 PM

>     > To: Fischetti, Antonio <antonio.fischetti@intel.com>;

>     > dev@openvswitch.org

>     > Subject: Re: [ovs-dev] [PATCH v3 2/5] conntrack: add commands to

> r/w CT

>     > parameters.

>     >

>     > One extra note inline

>     >

>     > Thanks Darrell

>     >

>     > On 12/11/17, 8:35 AM, "Darrell Ball" <dball@vmware.com> wrote:

>     >

>     >     Thanks Antonio for doing all this and pushing it forward.

>     >

>     >     Regarding patches 2-4:

>     >

>     >     I understand you want to save some code for various possible

> set and

>     > get operations.

>     >     The prior art for these commands is however not generic set

> and get

>     > commands.

>     >     Sometimes, we have specific commands that can take different

> numbers

>     > of arguments but

>     >     those are specific and the context is clear.

>     >     If we take some examples, we might see there is an issue with

> the

>     > approach here.

>     >     Take per zone limits as one example, which is an internal

>     > requirement that we are working on

>     >     across datapaths.

>     >     This is a set operation with 2 arguments plus the datapath = 3

>     > arguments.

>     >     This won’t work with the generic set functions here.

>     >     The corresponding get will not work either.

>     >     Trying to make this work generically will get pretty messy and

> the

>     > parameter validation error prone.

>     >

>     >     I would like to propose an alternative (a simple one; also

> with code

>     > consolidation and more error checking) below:

>     >     Please let me know what you think.

>     >

>     >     Thanks Darrell

>     >

>     >

> ///////////////////////////////////////////////////////////////

>     >

>     >     diff --git a/lib/conntrack.c b/lib/conntrack.c

>     >     index f5a3aa9..c8ad548 100644

>     >     --- a/lib/conntrack.c

>     >     +++ b/lib/conntrack.c

>     >     @@ -2485,6 +2485,27 @@ conntrack_flush(struct conntrack *ct,

> const

>     > uint16_t *zone)

>     >          return 0;

>     >      }

>     >

>     >     +int

>     >     +conntrack_set_maxconns(struct conntrack *ct, uint32_t

> maxconns)

>     >     +{

>     >     +    atomic_init(&ct->n_conn_limit, maxconns);

>     >     +    return 0;

>     >     +}

>     >     +

>     >     +int

>     >     +conntrack_get_maxconns(struct conntrack *ct, uint32_t

> *maxconns)

>     >     +{

>     >     +    atomic_read_relaxed(&ct->n_conn_limit, maxconns);

>     >     +    return 0;

>     >     +}

>     >     +

>     >     +int

>     >     +conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns)

>     >     +{

>     >     +    *nconns = atomic_count_get(&ct->n_conn);

>     >     +    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..8652724 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_maxconns(struct conntrack *ct, uint32_t

>     > maxconns);

>     >     +int conntrack_get_maxconns(struct conntrack *ct, uint32_t

>     > *maxconns);

>     >     +int conntrack_get_nconns(struct conntrack *ct, uint32_t

> *nconns);

>     >      ^L

>     >      /* '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 239c848..5fa3a97 100644

>     >     --- a/lib/ct-dpif.c

>     >     +++ b/lib/ct-dpif.c

>     >     @@ -140,6 +140,30 @@ ct_dpif_flush(struct dpif *dpif, const

> uint16_t

>     > *zone,

>     >                  : EOPNOTSUPP);

>     >      }

>     >

>     >     +int

>     >     +ct_dpif_set_maxconns(struct dpif *dpif, uint32_t maxconns)

>     >     +{

>     >     +    return (dpif->dpif_class->ct_set_maxconns

>     >     +            ? dpif->dpif_class->ct_set_maxconns(dpif,

> maxconns)

>     >     +            : EOPNOTSUPP);

>     >     +}

>     >     +

>     >     +int

>     >     +ct_dpif_get_maxconns(struct dpif *dpif, uint32_t *maxconns)

>     >     +{

>     >     +    return (dpif->dpif_class->ct_get_maxconns

>     >     +            ? dpif->dpif_class->ct_get_maxconns(dpif,

> maxconns)

>     >     +            : EOPNOTSUPP);

>     >     +}

>     >     +

>     >     +int

>     >     +ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)

>     >     +{

>     >     +    return (dpif->dpif_class->ct_get_nconns

>     >     +            ? dpif->dpif_class->ct_get_nconns(dpif, nconns)

>     >     +            : EOPNOTSUPP);

>     >     +}

>     >     +

>     >      void

>     >      ct_dpif_entry_uninit(struct ct_dpif_entry *entry)

>     >      {

>     >     diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h

>     >     index 5e2de53..09e7698 100644

>     >     --- a/lib/ct-dpif.h

>     >     +++ b/lib/ct-dpif.h

>     >     @@ -197,6 +197,9 @@ 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,

>     >                        const struct ct_dpif_tuple *);

>     >     +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);

>     >      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 a28ded9..215d1c3 100644

>     >     --- a/lib/dpctl.c

>     >     +++ b/lib/dpctl.c

>     >     @@ -1654,6 +1654,96 @@ dpctl_ct_bkts(int argc, const char

> *argv[],

>     >          return error;

>     >      }

>     >      ^L

>     >     +static int

>     >     +dpctl_ct_open_dp(int argc, const char *argv[],

>     >     +                 struct dpctl_params *dpctl_p, struct dpif

> **dpif)

>     >     +{

>     >     +    int error = 0;

>     >     +    /* The datapath name is not a mandatory parameter for

> this

>     > command.

>     >     +     * If it is not specified - so argc < 3 - we retrieve it

> from

>     > the

>     >     +     * current setup, assuming only one exists. */

>     >     +    char *dpname = argc == 3 ? xstrdup(argv[1]) :

>     > get_one_dp(dpctl_p);

>     >

>     >

>     > [Darrell] The ‘3’ here would be replaced with a parameter

> representing

>     > the number

>     >               of parameters expected when the dp is included in

> the

>     > parameter list.

>     >

>     >

>     >     +    if (!dpname) {

>     >     +        error = EINVAL;

>     >     +        dpctl_error(dpctl_p, error, "datapath not found");

>     >     +    } else {

>     >     +        error = parsed_dpif_open(dpname, false, dpif);

>     >     +        free(dpname);

>     >     +        if (error) {

>     >     +            dpctl_error(dpctl_p, error, "opening datapath");

>     >     +        }

>     >     +    }

>     >     +    return error;

>     >     +}

>     >     +

>     >     +static int

>     >     +dpctl_ct_set_maxconns(int argc, const char *argv[],

>     >     +                      struct dpctl_params *dpctl_p)

>     >     +{

>     >     +    struct dpif *dpif;

>     >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     >     +    if (!error) {

>     >     +        uint32_t maxconns;

>     >     +        if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) {

>     >     +            error = ct_dpif_set_maxconns(dpif, maxconns);

>     >     +            dpif_close(dpif);

>     >     +

>     >     +            if (!error) {

>     >     +                dpctl_print(dpctl_p, "Ok");

>     >     +            } else {

>     >     +                dpctl_error(dpctl_p, error, "ct set maxconns

>     > failed");

>     >     +            }

>     >     +        } else {

>     >     +            error = EINVAL;

>     >     +            dpctl_error(dpctl_p, error, "maxconns missing or

>     > malformed");

>     >     +        }

>     >     +    }

>     >     +

>     >     +    return error;

>     >     +}

>     >     +

>     >     +static int

>     >     +dpctl_ct_get_maxconns(int argc, const char *argv[],

>     >     +                    struct dpctl_params *dpctl_p)

>     >     +{

>     >     +    struct dpif *dpif;

>     >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     >     +    if (!error) {

>     >     +        uint32_t maxconns;

>     >     +        error = ct_dpif_get_maxconns(dpif, &maxconns);

>     >     +        dpif_close(dpif);

>     >     +

>     >     +        if (!error) {

>     >     +            dpctl_print(dpctl_p, "%u\n", maxconns);

>     >     +        } else {

>     >     +            dpctl_error(dpctl_p, error, "maxconns could not

> be

>     > retrieved");

>     >     +        }

>     >     +    }

>     >     +

>     >     +    return error;

>     >     +}

>     >     +

>     >     +static int

>     >     +dpctl_ct_get_nconns(int argc, const char *argv[],

>     >     +                    struct dpctl_params *dpctl_p)

>     >     +{

>     >     +    struct dpif *dpif;

>     >     +    int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif);

>     >     +    if (!error) {

>     >     +        uint32_t nconns;

>     >     +        error = ct_dpif_get_nconns(dpif, &nconns);

>     >     +        dpif_close(dpif);

>     >     +

>     >     +        if (!error) {

>     >     +            dpctl_print(dpctl_p, "%u\n", nconns);

>     >     +        } else {

>     >     +            dpctl_error(dpctl_p, error, "nconns could not be

>     > retrieved");

>     >     +        }

>     >     +    }

>     >     +

>     >     +    return error;

>     >     +}

>     >     +

>     >      /* Undocumented commands for unit testing. */

>     >

>     >      static int

>     >     @@ -1950,6 +2040,9 @@ 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-maxconns", "[dp] maxconns", 1, 2,

>     > dpctl_ct_set_maxconns, DP_RW },

>     >     +    { "ct-get-maxconns", "[dp] maxconns", 1, 2,

>     > dpctl_ct_get_maxconns, DP_RO },

>     >     +    { "ct-get-nconns", "[dp] nconns", 1, 2,

> dpctl_ct_get_nconns,

>     > 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 b1ef9a6..9432859 100644

>     >     --- a/lib/dpif-netdev.c

>     >     +++ b/lib/dpif-netdev.c

>     >     @@ -5745,6 +5745,30 @@ dpif_netdev_ct_flush(struct dpif *dpif,

> const

>     > uint16_t *zone,

>     >          return conntrack_flush(&dp->conntrack, zone);

>     >      }

>     >

>     >     +static int

>     >     +dpif_netdev_ct_set_maxconns(struct dpif *dpif, uint32_t

> maxconns)

>     >     +{

>     >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     >     +

>     >     +    return conntrack_set_maxconns(&dp->conntrack, maxconns);

>     >     +}

>     >     +

>     >     +static int

>     >     +dpif_netdev_ct_get_maxconns(struct dpif *dpif, uint32_t

> *maxconns)

>     >     +{

>     >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     >     +

>     >     +    return conntrack_get_nconns(&dp->conntrack, maxconns);

>     >     +}

>     >     +

>     >     +static int

>     >     +dpif_netdev_ct_get_nconns(struct dpif *dpif, uint32_t

> *nconns)

>     >     +{

>     >     +    struct dp_netdev *dp = get_dp_netdev(dpif);

>     >     +

>     >     +    return conntrack_get_nconns(&dp->conntrack, nconns);

>     >     +}

>     >     +

>     >      const struct dpif_class dpif_netdev_class = {

>     >          "netdev",

>     >          dpif_netdev_init,

>     >     @@ -5790,6 +5814,9 @@ 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_maxconns,

>     >     +    dpif_netdev_ct_get_maxconns,

>     >     +    dpif_netdev_ct_get_nconns,

>     >          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 3f76128..f8d75eb 100644

>     >     --- a/lib/dpif-netlink.c

>     >     +++ b/lib/dpif-netlink.c

>     >     @@ -2989,6 +2989,9 @@ const struct dpif_class

> dpif_netlink_class = {

>     >          dpif_netlink_ct_dump_next,

>     >          dpif_netlink_ct_dump_done,

>     >          dpif_netlink_ct_flush,

>     >     +    NULL,                       /* ct_set_maxconns */

>     >     +    NULL,                       /* ct_get_maxconns */

>     >     +    NULL,                       /* ct_get_nconns */

>     >          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 947bf5e..62b3598 100644

>     >     --- a/lib/dpif-provider.h

>     >     +++ b/lib/dpif-provider.h

>     >     @@ -437,6 +437,12 @@ struct dpif_class {

>     >           *     (zone 0). */

>     >          int (*ct_flush)(struct dpif *, const uint16_t *zone,

>     >                          const struct ct_dpif_tuple *tuple);

>     >     +    /* Set max connections allowed. */

>     >     +    int (*ct_set_maxconns)(struct dpif *, uint32_t maxconns);

>     >     +    /* Get max connections allowed. */

>     >     +    int (*ct_get_maxconns)(struct dpif *, uint32_t

> *maxconns);

>     >     +    /* Get number of connections tracked. */

>     >     +    int (*ct_get_nconns)(struct dpif *, uint32_t *nconns);

>     >

>     >          /* Meters */

>     >

>     >

>     >

> ///////////////////////////////////////////////////////////////

>     >

>     >

>     >

>     >     On 10/13/17, 1:27 AM, "ovs-dev-bounces@openvswitch.org on

> behalf of

>     > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on

> behalf

>     > of antonio.fischetti@intel.com> wrote:

>     >

>     >         Add infrastructure to implement:

>     >          - dpctl/ct-get-glbl-cfg to read a current value of

> available

>     >            conntrack parameters.

>     >          - dpctl/ct-set-glbl-cfg to set a value to the available

>     > conntrack

>     >            parameters.

>     >

>     >         CC: Darrell Ball <dlu998@gmail.com>

>     >         CC: Kevin Traynor <ktraynor@redhat.com>

>     >         Signed-off-by: Antonio Fischetti

> <antonio.fischetti@intel.com>

>     >         ---

>     >          lib/conntrack.c     | 60

>     > ++++++++++++++++++++++++++++++++++++++++++

>     >          lib/conntrack.h     |  3 +++

>     >          lib/ct-dpif.c       | 28 ++++++++++++++++++++

>     >          lib/ct-dpif.h       |  2 ++

>     >          lib/dpctl.c         | 76

>     > +++++++++++++++++++++++++++++++++++++++++++++++++++++

>     >          lib/dpif-netdev.c   | 19 ++++++++++++++

>     >          lib/dpif-netlink.c  |  2 ++

>     >          lib/dpif-provider.h |  4 +++

>     >          8 files changed, 194 insertions(+)

>     >

>     >         diff --git a/lib/conntrack.c b/lib/conntrack.c

>     >         index e555b55..29889c9 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_cfg_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);

>     >         @@ -2485,6 +2492,59 @@ conntrack_flush(struct conntrack

> *ct,

>     > const uint16_t *zone)

>     >              return 0;

>     >          }

>     >

>     >         +/* List of parameters that can be read/written at run-

> time. */

>     >         +struct ct_cfg_params cfg_params[] = {};

>     >         +

>     >         +int

>     >         +conntrack_set_param(struct conntrack *ct,

>     >         +        const char *set_param)

>     >         +{

>     >         +    uint32_t max_conn;

>     >         +    char buf[16] = "";

>     >         +

>     >         +    /* Check if the specified param can be managed. */

>     >         +    for (int i = 0; i < sizeof(cfg_params) /

> sizeof(struct

>     > ct_cfg_params);

>     >         +             i++) {

>     >         +        if (!strncmp(set_param, cfg_params[i].cli,

>     >         +                strlen(cfg_params[i].cli))) {

>     >         +            ovs_strzcpy(buf, cfg_params[i].cli,

> sizeof(buf) -

>     > 1);

>     >         +            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 -

>     > strlen(buf));

>     >         +            if (ovs_scan(set_param, buf, &max_conn)) {

>     >         +                return (cfg_params[i].wr

>     >         +                        ? cfg_params[i].wr(ct, max_conn)

>     >         +                        : EOPNOTSUPP);

>     >         +            } else {

>     >         +                return EINVAL;

>     >         +            }

>     >         +        }

>     >         +    }

>     >         +    /* The entered param didn't match any in the list. */

>     >         +    VLOG_WARN("%s parameter is not managed by this

> command.",

>     > set_param);

>     >         +

>     >         +    return EINVAL;

>     >         +}

>     >         +

>     >         +int

>     >         +conntrack_get_param(struct conntrack *ct,

>     >         +        const char *get_param, uint32_t *val)

>     >         +{

>     >         +    /* Check if the specified param can be managed. */

>     >         +    for (int i = 0; i < sizeof(cfg_params) /

> sizeof(struct

>     > ct_cfg_params);

>     >         +             i++) {

>     >         +        if (!strncmp(get_param, cfg_params[i].cli,

>     >         +                strlen(cfg_params[i].cli))) {

>     >         +

>     >         +            return (cfg_params[i].rd

>     >         +                    ? cfg_params[i].rd(ct, val)

>     >         +                    : EOPNOTSUPP);

>     >         +        }

>     >         +    }

>     >         +    /* The entered param didn't match any in the list. */

>     >         +    VLOG_WARN("%s parameter is not managed by this

> command.",

>     > get_param);

>     >         +

>     >         +    return EINVAL;

>     >         +}

>     >         +

>     >          /* 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 b6eecf0..5b02951 100644

>     >         --- a/lib/dpctl.c

>     >         +++ b/lib/dpctl.c

>     >         @@ -1589,6 +1589,80 @@ 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;

>     >         +

>     >         +    /* The datapath name is not a mandatory parameter for

> this

>     > command.

>     >         +     * If it is not specified - so argc < 3 - we retrieve

> it

>     > from the

>     >         +     * current setup, assuming only one exists. */

>     >         +    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]);

>     >         +

>     >         +    if (!error) {

>     >         +        dpctl_print(dpctl_p, "Ok");

>     >         +    } else {

>     >         +        dpctl_print(dpctl_p, "CT set global cfg failed

> (%s)",

>     >         +                        ovs_strerror(error));

>     >         +    }

>     >         +

>     >         +    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;

>     >         +

>     >         +    /* The datapath name is not a mandatory parameter for

> this

>     > command.

>     >         +     * If it is not specified - so argc < 3 - we retrieve

> it

>     > from the

>     >         +     * current setup, assuming only one exists. */

>     >         +    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],

>     > &param_val);

>     >         +

>     >         +    if (!error) {

>     >         +        dpctl_print(dpctl_p, "Current value: %d",

> param_val);

>     >         +    } else {

>     >         +        dpctl_print(dpctl_p, "CT get global cfg failed

> (%s)",

>     >         +                        ovs_strerror(error));

>     >         +    }

>     >         +

>     >         +    dpif_close(dpif);

>     >         +

>     >         +    return error;

>     >         +}

>     >         +

>     >          ?

>     >          /* Undocumented commands for unit testing. */

>     >

>     >         @@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2,

>     > dpctl_ct_set_param, DP_RW },

>     >         +    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644

>     >         --- a/lib/dpif-netdev.c

>     >         +++ b/lib/dpif-netdev.c

>     >         @@ -5721,6 +5721,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,

>     >         @@ -5766,6 +5783,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@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=m6PHM-SNZk7M88zlL6EQWY-S5kl3XTbB-YNfA-

> yk7xE&s=u0Gt2hVhSfd4NYQ0-

>     > mRy8r1P_U_vFyUqgE_Xd6rC9Qk&e=

>     >

>     >

>     >

>     >

>     >

>     >

>     >

>     >

>     >

> 

>
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index e555b55..29889c9 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_cfg_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);
@@ -2485,6 +2492,59 @@  conntrack_flush(struct conntrack *ct, const uint16_t *zone)
     return 0;
 }
 
+/* List of parameters that can be read/written at run-time. */
+struct ct_cfg_params cfg_params[] = {};
+
+int
+conntrack_set_param(struct conntrack *ct,
+        const char *set_param)
+{
+    uint32_t max_conn;
+    char buf[16] = "";
+
+    /* Check if the specified param can be managed. */
+    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
+             i++) {
+        if (!strncmp(set_param, cfg_params[i].cli,
+                strlen(cfg_params[i].cli))) {
+            ovs_strzcpy(buf, cfg_params[i].cli, sizeof(buf) - 1);
+            strncat(buf, "=%"SCNu32, sizeof(buf) - 1 - strlen(buf));
+            if (ovs_scan(set_param, buf, &max_conn)) {
+                return (cfg_params[i].wr
+                        ? cfg_params[i].wr(ct, max_conn)
+                        : EOPNOTSUPP);
+            } else {
+                return EINVAL;
+            }
+        }
+    }
+    /* The entered param didn't match any in the list. */
+    VLOG_WARN("%s parameter is not managed by this command.", set_param);
+
+    return EINVAL;
+}
+
+int
+conntrack_get_param(struct conntrack *ct,
+        const char *get_param, uint32_t *val)
+{
+    /* Check if the specified param can be managed. */
+    for (int i = 0; i < sizeof(cfg_params) / sizeof(struct ct_cfg_params);
+             i++) {
+        if (!strncmp(get_param, cfg_params[i].cli,
+                strlen(cfg_params[i].cli))) {
+
+            return (cfg_params[i].rd
+                    ? cfg_params[i].rd(ct, val)
+                    : EOPNOTSUPP);
+        }
+    }
+    /* The entered param didn't match any in the list. */
+    VLOG_WARN("%s parameter is not managed by this command.", get_param);
+
+    return EINVAL;
+}
+
 /* 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 b6eecf0..5b02951 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1589,6 +1589,80 @@  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;
+
+    /* The datapath name is not a mandatory parameter for this command.
+     * If it is not specified - so argc < 3 - we retrieve it from the
+     * current setup, assuming only one exists. */
+    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]);
+
+    if (!error) {
+        dpctl_print(dpctl_p, "Ok");
+    } else {
+        dpctl_print(dpctl_p, "CT set global cfg failed (%s)",
+                        ovs_strerror(error));
+    }
+
+    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;
+
+    /* The datapath name is not a mandatory parameter for this command.
+     * If it is not specified - so argc < 3 - we retrieve it from the
+     * current setup, assuming only one exists. */
+    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], &param_val);
+
+    if (!error) {
+        dpctl_print(dpctl_p, "Current value: %d", param_val);
+    } else {
+        dpctl_print(dpctl_p, "CT get global cfg failed (%s)",
+                        ovs_strerror(error));
+    }
+
+    dpif_close(dpif);
+
+    return error;
+}
+
 
 /* Undocumented commands for unit testing. */
 
@@ -1885,6 +1959,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-glbl-cfg", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
+    { "ct-get-glbl-cfg", "[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 d5eb830..4c0e7cf 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5721,6 +5721,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,
@@ -5766,6 +5783,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 */