diff mbox series

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

Message ID 1505730091-19042-1-git-send-email-antonio.fischetti@intel.com
State Changes Requested
Delegated to: Darrell Ball
Headers show
Series [ovs-dev,1/5] conntrack: add commands to r/w conntrack parameters. | expand

Commit Message

Fischetti, Antonio Sept. 18, 2017, 10:21 a.m. UTC 
Add infrastructure to implement:
 - dpctl/ct-get to read a current value of available
   conntrack parameters.
 - dpctl/ct-set to set a value to the available conntrack
   parameters.

Add dpctl/ct-get to read current values of conntrack
parameters.
Add dpctl/ct-set to set a value to conntrack parameters.

Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/conntrack.c     | 67 +++++++++++++++++++++++++++++++++++++++++
 lib/conntrack.h     |  3 ++
 lib/ct-dpif.c       | 28 ++++++++++++++++++
 lib/ct-dpif.h       |  2 ++
 lib/dpctl.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/dpif-netdev.c   | 19 ++++++++++++
 lib/dpif-netlink.c  |  2 ++
 lib/dpif-provider.h |  4 +++
 8 files changed, 210 insertions(+)

Comments

Darrell Ball Sept. 19, 2017, 7:50 a.m. UTC | #1 
Thanks for working on this Antonio

Few initial comments; in some cases, I did not repeat the same comment.


On 9/18/17, 3:22 AM, "ovs-dev-bounces@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 to read a current value of available
       conntrack parameters.
     - dpctl/ct-set to set a value to the available conntrack
       parameters.
    
    Add dpctl/ct-get to read current values of conntrack
    parameters.
    Add dpctl/ct-set to set a value to conntrack parameters.
    
    Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    ---
     lib/conntrack.c     | 67 +++++++++++++++++++++++++++++++++++++++++
     lib/conntrack.h     |  3 ++
     lib/ct-dpif.c       | 28 ++++++++++++++++++
     lib/ct-dpif.h       |  2 ++
     lib/dpctl.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++
     lib/dpif-netdev.c   | 19 ++++++++++++
     lib/dpif-netlink.c  |  2 ++
     lib/dpif-provider.h |  4 +++
     8 files changed, 210 insertions(+)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index 419cb1d..0642cc8 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -67,6 +67,13 @@ enum ct_alg_mode {
         CT_TFTP_MODE,
     };
     
    +/* Variable to manage read/write on CT parameters. */
    +struct ct_wk_params {

[Darrell]
Somehow, I don’t get the ‘_wk_’
How about ‘cfg’

    +    char *cli;      /* Parameter name in human format. */
    +    int (*wr)(struct conntrack *, uint32_t);
    +    int (*rd)(struct conntrack *, uint32_t *);
    +};
    +
     static bool conn_key_extract(struct conntrack *, struct dp_packet *,
                                  ovs_be16 dl_type, struct conn_lookup_ctx *,
                                  uint16_t zone);
    @@ -2391,6 +2398,66 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
         return 0;
     }
     
    +/* List of parameters that can be read/written at run-time. */
    +struct ct_wk_params wk_params[] = {};
    +
    +int
    +conntrack_set_param(struct conntrack *ct,
    +        const char *set_param)
    +{
    +    bool valid_param = false;
    +    uint32_t max_conn;
    +    char bfr[16] = "";

[Darrell]
bfr ?
could we use a few more letters ?

    +
    +    /* Check if the specified param can be managed. */
    +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
    +        if (!strncmp(set_param, wk_params[i].cli,
    +                strlen(wk_params[i].cli))) {
    +            valid_param = true;
    +            ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1);
    +            strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr));
    +            if (ovs_scan(set_param, bfr, &max_conn)) {
    +                return (wk_params[i].wr
    +                        ? wk_params[i].wr(ct, max_conn)
    +                        : EOPNOTSUPP);
    +            } else {
    +                return EINVAL;
    +            }
    +        }
    +    }

[Darrell] If we reach here, then won’t valid_param be false since we otherwise returned.


    +    if (!valid_param) {
    +        VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param);

[Darrell] VLOG_WARN ?
   ‘PARAM=NUMBER’ capitalization ?
Could we use a sentence or sentence fragment ?

    +        return EINVAL;
    +    }
    +
    +    return 0;
    +}
    +
    +int
    +conntrack_get_param(struct conntrack *ct,
    +        const char *get_param, uint32_t *val)
    +{
    +    bool valid_param = false;
    +
    +    /* Check if the specified param can be managed. */
    +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
    +        if (!strncmp(get_param, wk_params[i].cli,
    +                strlen(wk_params[i].cli))) {
    +            valid_param = true;
    +
    +            return (wk_params[i].rd
    +                    ? wk_params[i].rd(ct, val)
    +                    : EOPNOTSUPP);
    +        }
    +    }

[Darrell] If we reach here, then won’t valid_param be false since we otherwise returned.

    +    if (!valid_param) {
    +        VLOG_DBG("%s: expected a valid PARAM", get_param);

[Darrell] Why is ‘PARAM’ capitalized
VLOG_WARN ?


    +        return EINVAL;
    +    }
    +
    +    return 0;
    +}
    +
     /* This function must be called with the ct->resources read lock taken. */
     static struct alg_exp_node *
     expectation_lookup(struct hmap *alg_expectations,
    diff --git a/lib/conntrack.h b/lib/conntrack.h
    index fbeef1c..4eb9a9a 100644
    --- a/lib/conntrack.h
    +++ b/lib/conntrack.h
    @@ -114,6 +114,9 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *);
     int conntrack_dump_done(struct conntrack_dump *);
     
     int conntrack_flush(struct conntrack *, const uint16_t *zone);
    +int conntrack_set_param(struct conntrack *, const char *set_param);
    +int conntrack_get_param(struct conntrack *, const char *get_param,
    +                        uint32_t *val);
     ?
     /* 'struct ct_lock' is a wrapper for an adaptive mutex.  It's useful to try
      * different types of locks (e.g. spinlocks) */
    diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
    index c79e69e..599bc57 100644
    --- a/lib/ct-dpif.c
    +++ b/lib/ct-dpif.c
    @@ -127,6 +127,34 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
                 : EOPNOTSUPP);
     }
     
    +int
    +ct_dpif_set_param(struct dpif *dpif, const char *set_param)
    +{
    +    if (!set_param) {
    +        VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif));
    +        return EINVAL;
    +    }
    +    VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param);
    +
    +    return (dpif->dpif_class->ct_set_param
    +            ? dpif->dpif_class->ct_set_param(dpif, set_param)
    +            : EOPNOTSUPP);
    +}
    +
    +int
    +ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val)
    +{
    +    if (!get_param) {
    +        VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif));
    +        return EINVAL;
    +    }
    +    VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param);
    +
    +    return (dpif->dpif_class->ct_get_param
    +            ? dpif->dpif_class->ct_get_param(dpif, get_param, val)
    +            : EOPNOTSUPP);
    +}
    +
     void
     ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
     {
    diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
    index d5f9661..92ce3e9 100644
    --- a/lib/ct-dpif.h
    +++ b/lib/ct-dpif.h
    @@ -196,6 +196,8 @@ int ct_dpif_dump_start(struct dpif *, struct ct_dpif_dump_state **,
     int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry *);
     int ct_dpif_dump_done(struct ct_dpif_dump_state *);
     int ct_dpif_flush(struct dpif *, const uint16_t *zone);
    +int ct_dpif_set_param(struct dpif *dpif, const char *set_param);
    +int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val);
     void ct_dpif_entry_uninit(struct ct_dpif_entry *);
     void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
                               bool verbose, bool print_stats);
    diff --git a/lib/dpctl.c b/lib/dpctl.c
    index 8951d6e..2a4a924 100644
    --- a/lib/dpctl.c
    +++ b/lib/dpctl.c
    @@ -1563,6 +1563,89 @@ dpctl_ct_bkts(int argc, const char *argv[],
         free(conn_per_bkts);
         return error;
     }
    +
    +static int
    +dpctl_ct_set_param(int argc, const char *argv[],
    +        struct dpctl_params *dpctl_p)
    +{
    +    struct dpif *dpif;
    +    char *name;
    +    int error;
    +
    +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);

[Darrell] Maybe a comment ?
enum for ‘3’ ?

    +    if (!name) {
    +        return EINVAL;
    +    }
    +    error = parsed_dpif_open(name, false, &dpif);
    +    free(name);
    +    if (error) {
    +        dpctl_error(dpctl_p, error, "opening datapath");
    +        return error;
    +    }
    +
    +    error = ct_dpif_set_param(dpif, argv[argc - 1]);
    +
    +    switch (error) {

[Darrell] ovs_strerror() ?

    +    case 0:
    +        dpctl_print(dpctl_p, "Ok");
    +        break;
    +    case EOPNOTSUPP:
    +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
    +        break;
    +    case EINVAL:
    +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
    +        break;
    +    default:
    +        dpctl_print(dpctl_p, "Error:%d", error);
    +        break;
    +    }
    +    dpif_close(dpif);
    +
    +    return error;
    +}
    +
    +static int
    +dpctl_ct_get_param(int argc, const char *argv[],
    +        struct dpctl_params *dpctl_p)
    +{
    +    struct dpif *dpif;
    +    uint32_t param_val;
    +    char *name;
    +    int error;
    +
    +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
    +    if (!name) {
    +        return EINVAL;
    +    }
    +    error = parsed_dpif_open(name, false, &dpif);
    +    free(name);
    +    if (error) {
    +        dpctl_error(dpctl_p, error, "opening datapath");
    +        return error;
    +    }
    +
    +    error = ct_dpif_get_param(dpif, argv[argc - 1], &param_val);
    +
    +    switch (error) {
    +    case 0:
    +        dpctl_print(dpctl_p, "Current value: %d", param_val);
    +        break;
    +    case EOPNOTSUPP:
    +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
    +        break;
    +    case EINVAL:
    +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
    +        break;
    +    default:
    +        dpctl_print(dpctl_p, "Error:%d", error);
    +        break;
    +    }
    +
    +    dpif_close(dpif);
    +
    +    return error;
    +}
    +
     ?
     /* Undocumented commands for unit testing. */
     
    @@ -1859,6 +1942,8 @@ static const struct dpctl_command all_commands[] = {
         { "ct-stats-show", "[dp] [zone=N] [verbose]",
           0, 3, dpctl_ct_stats_show, DP_RO },
         { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
    +    { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
    +    { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO },

[Darrell]
ct-set-glbl-cfg ?
ct-get-glbl-cfg ?


         { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
         { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
     
    diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
    index ca74df8..8fda2a9 100644
    --- a/lib/dpif-netdev.c
    +++ b/lib/dpif-netdev.c
    @@ -5689,6 +5689,23 @@ dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone)
         return conntrack_flush(&dp->conntrack, zone);
     }
     
    +static int
    +dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param)
    +{
    +    struct dp_netdev *dp = get_dp_netdev(dpif);
    +
    +    return conntrack_set_param(&dp->conntrack, set_param);
    +}
    +
    +static int
    +dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param,
    +                         uint32_t *val)
    +{
    +    struct dp_netdev *dp = get_dp_netdev(dpif);
    +
    +    return conntrack_get_param(&dp->conntrack, get_param, val);
    +}
    +
     const struct dpif_class dpif_netdev_class = {
         "netdev",
         dpif_netdev_init,
    @@ -5734,6 +5751,8 @@ const struct dpif_class dpif_netdev_class = {
         dpif_netdev_ct_dump_next,
         dpif_netdev_ct_dump_done,
         dpif_netdev_ct_flush,
    +    dpif_netdev_ct_set_param,
    +    dpif_netdev_ct_get_param,
         dpif_netdev_meter_get_features,
         dpif_netdev_meter_set,
         dpif_netdev_meter_get,
    diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
    index 29001fb..0945fad 100644
    --- a/lib/dpif-netlink.c
    +++ b/lib/dpif-netlink.c
    @@ -2986,6 +2986,8 @@ const struct dpif_class dpif_netlink_class = {
         dpif_netlink_ct_dump_next,
         dpif_netlink_ct_dump_done,
         dpif_netlink_ct_flush,
    +    NULL,                       /* ct_set_param */
    +    NULL,                       /* ct_get_param */
         dpif_netlink_meter_get_features,
         dpif_netlink_meter_set,
         dpif_netlink_meter_get,
    diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
    index 1d82a09..262b2e0 100644
    --- a/lib/dpif-provider.h
    +++ b/lib/dpif-provider.h
    @@ -427,6 +427,10 @@ struct dpif_class {
         /* Flushes the connection tracking tables. If 'zone' is not NULL,
          * only deletes connections in '*zone'. */
         int (*ct_flush)(struct dpif *, const uint16_t *zone);
    +    /* Set a value to a connection tracking working parameter. */
    +    int (*ct_set_param)(struct dpif *, const char *set_param);
    +    /* Read the current value of a connection tracking working parameter. */
    +    int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t *val);
     
         /* Meters */
     
    -- 
    2.4.11
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=LD6cChwv8eq9Z53TD21taZaO8qTUdd29sNOYJkq-Rxc&s=Vi5y2hCa2H_54KZLLC7_hBvH5E3bcpu2916yMH4lzbQ&e=
Kevin Traynor Sept. 19, 2017, 1:10 p.m. UTC | #2 
On 09/18/2017 11:21 AM, antonio.fischetti@intel.com wrote:
> Add infrastructure to implement:
>  - dpctl/ct-get to read a current value of available
>    conntrack parameters.
>  - dpctl/ct-set to set a value to the available conntrack
>    parameters.
> 
> Add dpctl/ct-get to read current values of conntrack
> parameters.
> Add dpctl/ct-set to set a value to conntrack parameters.
> 

Hi Antonio - The commit message doesn't tell why these are needed or
what use cases they will help with etc. Can you add something in a cover
letter or the commits?

thanks,
Kevin.

> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
>  lib/conntrack.c     | 67 +++++++++++++++++++++++++++++++++++++++++
>  lib/conntrack.h     |  3 ++
>  lib/ct-dpif.c       | 28 ++++++++++++++++++
>  lib/ct-dpif.h       |  2 ++
>  lib/dpctl.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dpif-netdev.c   | 19 ++++++++++++
>  lib/dpif-netlink.c  |  2 ++
>  lib/dpif-provider.h |  4 +++
>  8 files changed, 210 insertions(+)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 419cb1d..0642cc8 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -67,6 +67,13 @@ enum ct_alg_mode {
>      CT_TFTP_MODE,
>  };
>  
> +/* Variable to manage read/write on CT parameters. */
> +struct ct_wk_params {
> +    char *cli;      /* Parameter name in human format. */
> +    int (*wr)(struct conntrack *, uint32_t);
> +    int (*rd)(struct conntrack *, uint32_t *);
> +};
> +
>  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
>                               ovs_be16 dl_type, struct conn_lookup_ctx *,
>                               uint16_t zone);
> @@ -2391,6 +2398,66 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
>      return 0;
>  }
>  
> +/* List of parameters that can be read/written at run-time. */
> +struct ct_wk_params wk_params[] = {};
> +
> +int
> +conntrack_set_param(struct conntrack *ct,
> +        const char *set_param)
> +{
> +    bool valid_param = false;
> +    uint32_t max_conn;
> +    char bfr[16] = "";
> +
> +    /* Check if the specified param can be managed. */
> +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
> +        if (!strncmp(set_param, wk_params[i].cli,
> +                strlen(wk_params[i].cli))) {
> +            valid_param = true;
> +            ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1);
> +            strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr));
> +            if (ovs_scan(set_param, bfr, &max_conn)) {
> +                return (wk_params[i].wr
> +                        ? wk_params[i].wr(ct, max_conn)
> +                        : EOPNOTSUPP);
> +            } else {
> +                return EINVAL;
> +            }
> +        }
> +    }
> +    if (!valid_param) {
> +        VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param);
> +        return EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +int
> +conntrack_get_param(struct conntrack *ct,
> +        const char *get_param, uint32_t *val)
> +{
> +    bool valid_param = false;
> +
> +    /* Check if the specified param can be managed. */
> +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
> +        if (!strncmp(get_param, wk_params[i].cli,
> +                strlen(wk_params[i].cli))) {
> +            valid_param = true;
> +
> +            return (wk_params[i].rd
> +                    ? wk_params[i].rd(ct, val)
> +                    : EOPNOTSUPP);
> +        }
> +    }
> +    if (!valid_param) {
> +        VLOG_DBG("%s: expected a valid PARAM", get_param);
> +        return EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  /* This function must be called with the ct->resources read lock taken. */
>  static struct alg_exp_node *
>  expectation_lookup(struct hmap *alg_expectations,
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index fbeef1c..4eb9a9a 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -114,6 +114,9 @@ int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *);
>  int conntrack_dump_done(struct conntrack_dump *);
>  
>  int conntrack_flush(struct conntrack *, const uint16_t *zone);
> +int conntrack_set_param(struct conntrack *, const char *set_param);
> +int conntrack_get_param(struct conntrack *, const char *get_param,
> +                        uint32_t *val);
>  
>  /* 'struct ct_lock' is a wrapper for an adaptive mutex.  It's useful to try
>   * different types of locks (e.g. spinlocks) */
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index c79e69e..599bc57 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -127,6 +127,34 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
>              : EOPNOTSUPP);
>  }
>  
> +int
> +ct_dpif_set_param(struct dpif *dpif, const char *set_param)
> +{
> +    if (!set_param) {
> +        VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif));
> +        return EINVAL;
> +    }
> +    VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param);
> +
> +    return (dpif->dpif_class->ct_set_param
> +            ? dpif->dpif_class->ct_set_param(dpif, set_param)
> +            : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val)
> +{
> +    if (!get_param) {
> +        VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif));
> +        return EINVAL;
> +    }
> +    VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param);
> +
> +    return (dpif->dpif_class->ct_get_param
> +            ? dpif->dpif_class->ct_get_param(dpif, get_param, val)
> +            : EOPNOTSUPP);
> +}
> +
>  void
>  ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
>  {
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index d5f9661..92ce3e9 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -196,6 +196,8 @@ int ct_dpif_dump_start(struct dpif *, struct ct_dpif_dump_state **,
>  int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry *);
>  int ct_dpif_dump_done(struct ct_dpif_dump_state *);
>  int ct_dpif_flush(struct dpif *, const uint16_t *zone);
> +int ct_dpif_set_param(struct dpif *dpif, const char *set_param);
> +int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val);
>  void ct_dpif_entry_uninit(struct ct_dpif_entry *);
>  void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
>                            bool verbose, bool print_stats);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 8951d6e..2a4a924 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1563,6 +1563,89 @@ dpctl_ct_bkts(int argc, const char *argv[],
>      free(conn_per_bkts);
>      return error;
>  }
> +
> +static int
> +dpctl_ct_set_param(int argc, const char *argv[],
> +        struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    char *name;
> +    int error;
> +
> +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
> +    if (!name) {
> +        return EINVAL;
> +    }
> +    error = parsed_dpif_open(name, false, &dpif);
> +    free(name);
> +    if (error) {
> +        dpctl_error(dpctl_p, error, "opening datapath");
> +        return error;
> +    }
> +
> +    error = ct_dpif_set_param(dpif, argv[argc - 1]);
> +
> +    switch (error) {
> +    case 0:
> +        dpctl_print(dpctl_p, "Ok");
> +        break;
> +    case EOPNOTSUPP:
> +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
> +        break;
> +    case EINVAL:
> +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
> +        break;
> +    default:
> +        dpctl_print(dpctl_p, "Error:%d", error);
> +        break;
> +    }
> +    dpif_close(dpif);
> +
> +    return error;
> +}
> +
> +static int
> +dpctl_ct_get_param(int argc, const char *argv[],
> +        struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    uint32_t param_val;
> +    char *name;
> +    int error;
> +
> +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
> +    if (!name) {
> +        return EINVAL;
> +    }
> +    error = parsed_dpif_open(name, false, &dpif);
> +    free(name);
> +    if (error) {
> +        dpctl_error(dpctl_p, error, "opening datapath");
> +        return error;
> +    }
> +
> +    error = ct_dpif_get_param(dpif, argv[argc - 1], &param_val);
> +
> +    switch (error) {
> +    case 0:
> +        dpctl_print(dpctl_p, "Current value: %d", param_val);
> +        break;
> +    case EOPNOTSUPP:
> +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
> +        break;
> +    case EINVAL:
> +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
> +        break;
> +    default:
> +        dpctl_print(dpctl_p, "Error:%d", error);
> +        break;
> +    }
> +
> +    dpif_close(dpif);
> +
> +    return error;
> +}
> +
>  
>  /* Undocumented commands for unit testing. */
>  
> @@ -1859,6 +1942,8 @@ static const struct dpctl_command all_commands[] = {
>      { "ct-stats-show", "[dp] [zone=N] [verbose]",
>        0, 3, dpctl_ct_stats_show, DP_RO },
>      { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
> +    { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
> +    { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO },
>      { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
>      { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
>  
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index ca74df8..8fda2a9 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5689,6 +5689,23 @@ dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone)
>      return conntrack_flush(&dp->conntrack, zone);
>  }
>  
> +static int
> +dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param)
> +{
> +    struct dp_netdev *dp = get_dp_netdev(dpif);
> +
> +    return conntrack_set_param(&dp->conntrack, set_param);
> +}
> +
> +static int
> +dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param,
> +                         uint32_t *val)
> +{
> +    struct dp_netdev *dp = get_dp_netdev(dpif);
> +
> +    return conntrack_get_param(&dp->conntrack, get_param, val);
> +}
> +
>  const struct dpif_class dpif_netdev_class = {
>      "netdev",
>      dpif_netdev_init,
> @@ -5734,6 +5751,8 @@ const struct dpif_class dpif_netdev_class = {
>      dpif_netdev_ct_dump_next,
>      dpif_netdev_ct_dump_done,
>      dpif_netdev_ct_flush,
> +    dpif_netdev_ct_set_param,
> +    dpif_netdev_ct_get_param,
>      dpif_netdev_meter_get_features,
>      dpif_netdev_meter_set,
>      dpif_netdev_meter_get,
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 29001fb..0945fad 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2986,6 +2986,8 @@ const struct dpif_class dpif_netlink_class = {
>      dpif_netlink_ct_dump_next,
>      dpif_netlink_ct_dump_done,
>      dpif_netlink_ct_flush,
> +    NULL,                       /* ct_set_param */
> +    NULL,                       /* ct_get_param */
>      dpif_netlink_meter_get_features,
>      dpif_netlink_meter_set,
>      dpif_netlink_meter_get,
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 1d82a09..262b2e0 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -427,6 +427,10 @@ struct dpif_class {
>      /* Flushes the connection tracking tables. If 'zone' is not NULL,
>       * only deletes connections in '*zone'. */
>      int (*ct_flush)(struct dpif *, const uint16_t *zone);
> +    /* Set a value to a connection tracking working parameter. */
> +    int (*ct_set_param)(struct dpif *, const char *set_param);
> +    /* Read the current value of a connection tracking working parameter. */
> +    int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t *val);
>  
>      /* Meters */
>  
>
Fischetti, Antonio Sept. 20, 2017, 6:30 p.m. UTC | #3 
Thanks Darrell for your comments, I'll re-spin a V2 based on your feedback.
Other details inline below.

-Antonio

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

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

> Sent: Tuesday, September 19, 2017 8:50 AM

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

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

> parameters.

> 

> Thanks for working on this Antonio

> 

> Few initial comments; in some cases, I did not repeat the same comment.

> 

> 

> On 9/18/17, 3:22 AM, "ovs-dev-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 to read a current value of available

>        conntrack parameters.

>      - dpctl/ct-set to set a value to the available conntrack

>        parameters.

> 

>     Add dpctl/ct-get to read current values of conntrack

>     parameters.

>     Add dpctl/ct-set to set a value to conntrack parameters.

> 

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

>     ---

>      lib/conntrack.c     | 67 +++++++++++++++++++++++++++++++++++++++++

>      lib/conntrack.h     |  3 ++

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

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

>      lib/dpctl.c         | 85

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

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

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

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

>      8 files changed, 210 insertions(+)

> 

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

>     index 419cb1d..0642cc8 100644

>     --- a/lib/conntrack.c

>     +++ b/lib/conntrack.c

>     @@ -67,6 +67,13 @@ enum ct_alg_mode {

>          CT_TFTP_MODE,

>      };

> 

>     +/* Variable to manage read/write on CT parameters. */

>     +struct ct_wk_params {

> 

> [Darrell]

> Somehow, I don’t get the ‘_wk_’

> How about ‘cfg’


[Antonio] agree, ct_cfg_params sounds better.
I was using 'wk' as 'working'.

> 

>     +    char *cli;      /* Parameter name in human format. */

>     +    int (*wr)(struct conntrack *, uint32_t);

>     +    int (*rd)(struct conntrack *, uint32_t *);

>     +};

>     +

>      static bool conn_key_extract(struct conntrack *, struct dp_packet *,

>                                   ovs_be16 dl_type, struct conn_lookup_ctx *,

>                                   uint16_t zone);

>     @@ -2391,6 +2398,66 @@ conntrack_flush(struct conntrack *ct, const uint16_t

> *zone)

>          return 0;

>      }

> 

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

>     +struct ct_wk_params wk_params[] = {};

>     +

>     +int

>     +conntrack_set_param(struct conntrack *ct,

>     +        const char *set_param)

>     +{

>     +    bool valid_param = false;

>     +    uint32_t max_conn;

>     +    char bfr[16] = "";

> 

> [Darrell]

> bfr ?

> could we use a few more letters ?


[Antonio]
As it is a temp buffer I could rename it 'temp' or 'buf'?

> 

>     +

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

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

> i++) {

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

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

>     +            valid_param = true;

>     +            ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1);

>     +            strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr));

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

>     +                return (wk_params[i].wr

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

>     +                        : EOPNOTSUPP);

>     +            } else {

>     +                return EINVAL;

>     +            }

>     +        }

>     +    }

> 

> [Darrell] If we reach here, then won’t valid_param be false since we otherwise

> returned.


[Antonio] you're right, will fix.


> 

> 

>     +    if (!valid_param) {

>     +        VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param);

> 

> [Darrell] VLOG_WARN ?

>    ‘PARAM=NUMBER’ capitalization ?

> Could we use a sentence or sentence fragment ?


[Antonio] will change it to
  VLOG_WARN("%s parameter is not managed by this command.", set_param);

> 

>     +        return EINVAL;

>     +    }

>     +

>     +    return 0;

>     +}

>     +

>     +int

>     +conntrack_get_param(struct conntrack *ct,

>     +        const char *get_param, uint32_t *val)

>     +{

>     +    bool valid_param = false;

>     +

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

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

> i++) {

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

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

>     +            valid_param = true;

>     +

>     +            return (wk_params[i].rd

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

>     +                    : EOPNOTSUPP);

>     +        }

>     +    }

> 

> [Darrell] If we reach here, then won’t valid_param be false since we otherwise

> returned.


[Antonio] right, will fix.

> 

>     +    if (!valid_param) {

>     +        VLOG_DBG("%s: expected a valid PARAM", get_param);

> 

> [Darrell] Why is ‘PARAM’ capitalized

> VLOG_WARN ?


[Antonio] will change it to 
  VLOG_WARN("%s parameter is not managed by this command.", get_param);


> 

> 

>     +        return EINVAL;

>     +    }

>     +

>     +    return 0;

>     +}

>     +

>      /* This function must be called with the ct->resources read lock taken. */

>      static struct alg_exp_node *

>      expectation_lookup(struct hmap *alg_expectations,

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

>     index fbeef1c..4eb9a9a 100644

>     --- a/lib/conntrack.h

>     +++ b/lib/conntrack.h

>     @@ -114,6 +114,9 @@ int conntrack_dump_next(struct conntrack_dump *, struct

> ct_dpif_entry *);

>      int conntrack_dump_done(struct conntrack_dump *);

> 

>      int conntrack_flush(struct conntrack *, const uint16_t *zone);

>     +int conntrack_set_param(struct conntrack *, const char *set_param);

>     +int conntrack_get_param(struct conntrack *, const char *get_param,

>     +                        uint32_t *val);

>      ?

>      /* 'struct ct_lock' is a wrapper for an adaptive mutex.  It's useful to

> try

>       * different types of locks (e.g. spinlocks) */

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

>     index c79e69e..599bc57 100644

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

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

>     @@ -127,6 +127,34 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)

>                  : EOPNOTSUPP);

>      }

> 

>     +int

>     +ct_dpif_set_param(struct dpif *dpif, const char *set_param)

>     +{

>     +    if (!set_param) {

>     +        VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif));

>     +        return EINVAL;

>     +    }

>     +    VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param);

>     +

>     +    return (dpif->dpif_class->ct_set_param

>     +            ? dpif->dpif_class->ct_set_param(dpif, set_param)

>     +            : EOPNOTSUPP);

>     +}

>     +

>     +int

>     +ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val)

>     +{

>     +    if (!get_param) {

>     +        VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif));

>     +        return EINVAL;

>     +    }

>     +    VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param);

>     +

>     +    return (dpif->dpif_class->ct_get_param

>     +            ? dpif->dpif_class->ct_get_param(dpif, get_param, val)

>     +            : EOPNOTSUPP);

>     +}

>     +

>      void

>      ct_dpif_entry_uninit(struct ct_dpif_entry *entry)

>      {

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

>     index d5f9661..92ce3e9 100644

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

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

>     @@ -196,6 +196,8 @@ int ct_dpif_dump_start(struct dpif *, struct

> ct_dpif_dump_state **,

>      int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry

> *);

>      int ct_dpif_dump_done(struct ct_dpif_dump_state *);

>      int ct_dpif_flush(struct dpif *, const uint16_t *zone);

>     +int ct_dpif_set_param(struct dpif *dpif, const char *set_param);

>     +int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t

> *val);

>      void ct_dpif_entry_uninit(struct ct_dpif_entry *);

>      void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,

>                                bool verbose, bool print_stats);

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

>     index 8951d6e..2a4a924 100644

>     --- a/lib/dpctl.c

>     +++ b/lib/dpctl.c

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

>          free(conn_per_bkts);

>          return error;

>      }

>     +

>     +static int

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

>     +        struct dpctl_params *dpctl_p)

>     +{

>     +    struct dpif *dpif;

>     +    char *name;

>     +    int error;

>     +

>     +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);

> 

> [Darrell] Maybe a comment ?

> enum for ‘3’ ?


[Antonio] agree, will add a comment also on other lines where this 3 is
appearing.

> 

>     +    if (!name) {

>     +        return EINVAL;

>     +    }

>     +    error = parsed_dpif_open(name, false, &dpif);

>     +    free(name);

>     +    if (error) {

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

>     +        return error;

>     +    }

>     +

>     +    error = ct_dpif_set_param(dpif, argv[argc - 1]);

>     +

>     +    switch (error) {

> 

> [Darrell] ovs_strerror() ?


[Antonio] ok will change.


> 

>     +    case 0:

>     +        dpctl_print(dpctl_p, "Ok");

>     +        break;

>     +    case EOPNOTSUPP:

>     +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);

>     +        break;

>     +    case EINVAL:

>     +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);

>     +        break;

>     +    default:

>     +        dpctl_print(dpctl_p, "Error:%d", error);

>     +        break;

>     +    }

>     +    dpif_close(dpif);

>     +

>     +    return error;

>     +}

>     +

>     +static int

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

>     +        struct dpctl_params *dpctl_p)

>     +{

>     +    struct dpif *dpif;

>     +    uint32_t param_val;

>     +    char *name;

>     +    int error;

>     +

>     +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);

>     +    if (!name) {

>     +        return EINVAL;

>     +    }

>     +    error = parsed_dpif_open(name, false, &dpif);

>     +    free(name);

>     +    if (error) {

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

>     +        return error;

>     +    }

>     +

>     +    error = ct_dpif_get_param(dpif, argv[argc - 1], &param_val);

>     +

>     +    switch (error) {

>     +    case 0:

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

>     +        break;

>     +    case EOPNOTSUPP:

>     +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);

>     +        break;

>     +    case EINVAL:

>     +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);

>     +        break;

>     +    default:

>     +        dpctl_print(dpctl_p, "Error:%d", error);

>     +        break;

>     +    }

>     +

>     +    dpif_close(dpif);

>     +

>     +    return error;

>     +}

>     +

>      ?

>      /* Undocumented commands for unit testing. */

> 

>     @@ -1859,6 +1942,8 @@ static const struct dpctl_command all_commands[] = {

>          { "ct-stats-show", "[dp] [zone=N] [verbose]",

>            0, 3, dpctl_ct_stats_show, DP_RO },

>          { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },

>     +    { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },

>     +    { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO },

> 

> [Darrell]

> ct-set-glbl-cfg ?

> ct-get-glbl-cfg ?


[Antonio] ok will change


> 

> 

>          { "help", "", 0, INT_MAX, dpctl_help, DP_RO },

>          { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },

> 

>     diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c

>     index ca74df8..8fda2a9 100644

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

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

>     @@ -5689,6 +5689,23 @@ dpif_netdev_ct_flush(struct dpif *dpif, const

> uint16_t *zone)

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

>      }

> 

>     +static int

>     +dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param)

>     +{

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

>     +

>     +    return conntrack_set_param(&dp->conntrack, set_param);

>     +}

>     +

>     +static int

>     +dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param,

>     +                         uint32_t *val)

>     +{

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

>     +

>     +    return conntrack_get_param(&dp->conntrack, get_param, val);

>     +}

>     +

>      const struct dpif_class dpif_netdev_class = {

>          "netdev",

>          dpif_netdev_init,

>     @@ -5734,6 +5751,8 @@ const struct dpif_class dpif_netdev_class = {

>          dpif_netdev_ct_dump_next,

>          dpif_netdev_ct_dump_done,

>          dpif_netdev_ct_flush,

>     +    dpif_netdev_ct_set_param,

>     +    dpif_netdev_ct_get_param,

>          dpif_netdev_meter_get_features,

>          dpif_netdev_meter_set,

>          dpif_netdev_meter_get,

>     diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c

>     index 29001fb..0945fad 100644

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

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

>     @@ -2986,6 +2986,8 @@ const struct dpif_class dpif_netlink_class = {

>          dpif_netlink_ct_dump_next,

>          dpif_netlink_ct_dump_done,

>          dpif_netlink_ct_flush,

>     +    NULL,                       /* ct_set_param */

>     +    NULL,                       /* ct_get_param */

>          dpif_netlink_meter_get_features,

>          dpif_netlink_meter_set,

>          dpif_netlink_meter_get,

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

>     index 1d82a09..262b2e0 100644

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

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

>     @@ -427,6 +427,10 @@ struct dpif_class {

>          /* Flushes the connection tracking tables. If 'zone' is not NULL,

>           * only deletes connections in '*zone'. */

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

>     +    /* Set a value to a connection tracking working parameter. */

>     +    int (*ct_set_param)(struct dpif *, const char *set_param);

>     +    /* Read the current value of a connection tracking working parameter.

> */

>     +    int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t

> *val);

> 

>          /* Meters */

> 

>     --

>     2.4.11

> 

>     _______________________________________________

>     dev mailing list

>     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=LD6cChwv8eq9Z53TD21taZaO8qTUdd29sNOYJkq-

> Rxc&s=Vi5y2hCa2H_54KZLLC7_hBvH5E3bcpu2916yMH4lzbQ&e=

>
Fischetti, Antonio Sept. 20, 2017, 6:38 p.m. UTC | #4 
Sure Kevin, I'll add a cover letter to the new version I'm going to rework.
Basically these are two new commands to allow read/adjust some of the CT
configuration parameter. So that the user can try to find a better tuning
for his/her setup.

Thanks,
Antonio


> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday, September 19, 2017 2:11 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/5] conntrack: add commands to r/w conntrack
> parameters.
> 
> On 09/18/2017 11:21 AM, antonio.fischetti@intel.com wrote:
> > Add infrastructure to implement:
> >  - dpctl/ct-get to read a current value of available
> >    conntrack parameters.
> >  - dpctl/ct-set to set a value to the available conntrack
> >    parameters.
> >
> > Add dpctl/ct-get to read current values of conntrack
> > parameters.
> > Add dpctl/ct-set to set a value to conntrack parameters.
> >
> 
> Hi Antonio - The commit message doesn't tell why these are needed or
> what use cases they will help with etc. Can you add something in a cover
> letter or the commits?
> 
> thanks,
> Kevin.
> 
> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > ---
> >  lib/conntrack.c     | 67 +++++++++++++++++++++++++++++++++++++++++
> >  lib/conntrack.h     |  3 ++
> >  lib/ct-dpif.c       | 28 ++++++++++++++++++
> >  lib/ct-dpif.h       |  2 ++
> >  lib/dpctl.c         | 85
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/dpif-netdev.c   | 19 ++++++++++++
> >  lib/dpif-netlink.c  |  2 ++
> >  lib/dpif-provider.h |  4 +++
> >  8 files changed, 210 insertions(+)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 419cb1d..0642cc8 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -67,6 +67,13 @@ enum ct_alg_mode {
> >      CT_TFTP_MODE,
> >  };
> >
> > +/* Variable to manage read/write on CT parameters. */
> > +struct ct_wk_params {
> > +    char *cli;      /* Parameter name in human format. */
> > +    int (*wr)(struct conntrack *, uint32_t);
> > +    int (*rd)(struct conntrack *, uint32_t *);
> > +};
> > +
> >  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
> >                               ovs_be16 dl_type, struct conn_lookup_ctx *,
> >                               uint16_t zone);
> > @@ -2391,6 +2398,66 @@ conntrack_flush(struct conntrack *ct, const uint16_t
> *zone)
> >      return 0;
> >  }
> >
> > +/* List of parameters that can be read/written at run-time. */
> > +struct ct_wk_params wk_params[] = {};
> > +
> > +int
> > +conntrack_set_param(struct conntrack *ct,
> > +        const char *set_param)
> > +{
> > +    bool valid_param = false;
> > +    uint32_t max_conn;
> > +    char bfr[16] = "";
> > +
> > +    /* Check if the specified param can be managed. */
> > +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params);
> i++) {
> > +        if (!strncmp(set_param, wk_params[i].cli,
> > +                strlen(wk_params[i].cli))) {
> > +            valid_param = true;
> > +            ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1);
> > +            strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr));
> > +            if (ovs_scan(set_param, bfr, &max_conn)) {
> > +                return (wk_params[i].wr
> > +                        ? wk_params[i].wr(ct, max_conn)
> > +                        : EOPNOTSUPP);
> > +            } else {
> > +                return EINVAL;
> > +            }
> > +        }
> > +    }
> > +    if (!valid_param) {
> > +        VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param);
> > +        return EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int
> > +conntrack_get_param(struct conntrack *ct,
> > +        const char *get_param, uint32_t *val)
> > +{
> > +    bool valid_param = false;
> > +
> > +    /* Check if the specified param can be managed. */
> > +    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params);
> i++) {
> > +        if (!strncmp(get_param, wk_params[i].cli,
> > +                strlen(wk_params[i].cli))) {
> > +            valid_param = true;
> > +
> > +            return (wk_params[i].rd
> > +                    ? wk_params[i].rd(ct, val)
> > +                    : EOPNOTSUPP);
> > +        }
> > +    }
> > +    if (!valid_param) {
> > +        VLOG_DBG("%s: expected a valid PARAM", get_param);
> > +        return EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  /* This function must be called with the ct->resources read lock taken. */
> >  static struct alg_exp_node *
> >  expectation_lookup(struct hmap *alg_expectations,
> > diff --git a/lib/conntrack.h b/lib/conntrack.h
> > index fbeef1c..4eb9a9a 100644
> > --- a/lib/conntrack.h
> > +++ b/lib/conntrack.h
> > @@ -114,6 +114,9 @@ int conntrack_dump_next(struct conntrack_dump *, struct
> ct_dpif_entry *);
> >  int conntrack_dump_done(struct conntrack_dump *);
> >
> >  int conntrack_flush(struct conntrack *, const uint16_t *zone);
> > +int conntrack_set_param(struct conntrack *, const char *set_param);
> > +int conntrack_get_param(struct conntrack *, const char *get_param,
> > +                        uint32_t *val);
> >  

> >  /* 'struct ct_lock' is a wrapper for an adaptive mutex.  It's useful to try
> >   * different types of locks (e.g. spinlocks) */
> > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> > index c79e69e..599bc57 100644
> > --- a/lib/ct-dpif.c
> > +++ b/lib/ct-dpif.c
> > @@ -127,6 +127,34 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
> >              : EOPNOTSUPP);
> >  }
> >
> > +int
> > +ct_dpif_set_param(struct dpif *dpif, const char *set_param)
> > +{
> > +    if (!set_param) {
> > +        VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif));
> > +        return EINVAL;
> > +    }
> > +    VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param);
> > +
> > +    return (dpif->dpif_class->ct_set_param
> > +            ? dpif->dpif_class->ct_set_param(dpif, set_param)
> > +            : EOPNOTSUPP);
> > +}
> > +
> > +int
> > +ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val)
> > +{
> > +    if (!get_param) {
> > +        VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif));
> > +        return EINVAL;
> > +    }
> > +    VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param);
> > +
> > +    return (dpif->dpif_class->ct_get_param
> > +            ? dpif->dpif_class->ct_get_param(dpif, get_param, val)
> > +            : EOPNOTSUPP);
> > +}
> > +
> >  void
> >  ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> >  {
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index d5f9661..92ce3e9 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -196,6 +196,8 @@ int ct_dpif_dump_start(struct dpif *, struct
> ct_dpif_dump_state **,
> >  int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry *);
> >  int ct_dpif_dump_done(struct ct_dpif_dump_state *);
> >  int ct_dpif_flush(struct dpif *, const uint16_t *zone);
> > +int ct_dpif_set_param(struct dpif *dpif, const char *set_param);
> > +int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t
> *val);
> >  void ct_dpif_entry_uninit(struct ct_dpif_entry *);
> >  void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
> >                            bool verbose, bool print_stats);
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 8951d6e..2a4a924 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -1563,6 +1563,89 @@ dpctl_ct_bkts(int argc, const char *argv[],
> >      free(conn_per_bkts);
> >      return error;
> >  }
> > +
> > +static int
> > +dpctl_ct_set_param(int argc, const char *argv[],
> > +        struct dpctl_params *dpctl_p)
> > +{
> > +    struct dpif *dpif;
> > +    char *name;
> > +    int error;
> > +
> > +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
> > +    if (!name) {
> > +        return EINVAL;
> > +    }
> > +    error = parsed_dpif_open(name, false, &dpif);
> > +    free(name);
> > +    if (error) {
> > +        dpctl_error(dpctl_p, error, "opening datapath");
> > +        return error;
> > +    }
> > +
> > +    error = ct_dpif_set_param(dpif, argv[argc - 1]);
> > +
> > +    switch (error) {
> > +    case 0:
> > +        dpctl_print(dpctl_p, "Ok");
> > +        break;
> > +    case EOPNOTSUPP:
> > +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
> > +        break;
> > +    case EINVAL:
> > +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
> > +        break;
> > +    default:
> > +        dpctl_print(dpctl_p, "Error:%d", error);
> > +        break;
> > +    }
> > +    dpif_close(dpif);
> > +
> > +    return error;
> > +}
> > +
> > +static int
> > +dpctl_ct_get_param(int argc, const char *argv[],
> > +        struct dpctl_params *dpctl_p)
> > +{
> > +    struct dpif *dpif;
> > +    uint32_t param_val;
> > +    char *name;
> > +    int error;
> > +
> > +    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
> > +    if (!name) {
> > +        return EINVAL;
> > +    }
> > +    error = parsed_dpif_open(name, false, &dpif);
> > +    free(name);
> > +    if (error) {
> > +        dpctl_error(dpctl_p, error, "opening datapath");
> > +        return error;
> > +    }
> > +
> > +    error = ct_dpif_get_param(dpif, argv[argc - 1], &param_val);
> > +
> > +    switch (error) {
> > +    case 0:
> > +        dpctl_print(dpctl_p, "Current value: %d", param_val);
> > +        break;
> > +    case EOPNOTSUPP:
> > +        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
> > +        break;
> > +    case EINVAL:
> > +        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
> > +        break;
> > +    default:
> > +        dpctl_print(dpctl_p, "Error:%d", error);
> > +        break;
> > +    }
> > +
> > +    dpif_close(dpif);
> > +
> > +    return error;
> > +}
> > +
> >  

> >  /* Undocumented commands for unit testing. */
> >
> > @@ -1859,6 +1942,8 @@ static const struct dpctl_command all_commands[] = {
> >      { "ct-stats-show", "[dp] [zone=N] [verbose]",
> >        0, 3, dpctl_ct_stats_show, DP_RO },
> >      { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
> > +    { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
> > +    { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO },
> >      { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
> >      { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index ca74df8..8fda2a9 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -5689,6 +5689,23 @@ dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t
> *zone)
> >      return conntrack_flush(&dp->conntrack, zone);
> >  }
> >
> > +static int
> > +dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param)
> > +{
> > +    struct dp_netdev *dp = get_dp_netdev(dpif);
> > +
> > +    return conntrack_set_param(&dp->conntrack, set_param);
> > +}
> > +
> > +static int
> > +dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param,
> > +                         uint32_t *val)
> > +{
> > +    struct dp_netdev *dp = get_dp_netdev(dpif);
> > +
> > +    return conntrack_get_param(&dp->conntrack, get_param, val);
> > +}
> > +
> >  const struct dpif_class dpif_netdev_class = {
> >      "netdev",
> >      dpif_netdev_init,
> > @@ -5734,6 +5751,8 @@ const struct dpif_class dpif_netdev_class = {
> >      dpif_netdev_ct_dump_next,
> >      dpif_netdev_ct_dump_done,
> >      dpif_netdev_ct_flush,
> > +    dpif_netdev_ct_set_param,
> > +    dpif_netdev_ct_get_param,
> >      dpif_netdev_meter_get_features,
> >      dpif_netdev_meter_set,
> >      dpif_netdev_meter_get,
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index 29001fb..0945fad 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -2986,6 +2986,8 @@ const struct dpif_class dpif_netlink_class = {
> >      dpif_netlink_ct_dump_next,
> >      dpif_netlink_ct_dump_done,
> >      dpif_netlink_ct_flush,
> > +    NULL,                       /* ct_set_param */
> > +    NULL,                       /* ct_get_param */
> >      dpif_netlink_meter_get_features,
> >      dpif_netlink_meter_set,
> >      dpif_netlink_meter_get,
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 1d82a09..262b2e0 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -427,6 +427,10 @@ struct dpif_class {
> >      /* Flushes the connection tracking tables. If 'zone' is not NULL,
> >       * only deletes connections in '*zone'. */
> >      int (*ct_flush)(struct dpif *, const uint16_t *zone);
> > +    /* Set a value to a connection tracking working parameter. */
> > +    int (*ct_set_param)(struct dpif *, const char *set_param);
> > +    /* Read the current value of a connection tracking working parameter. */
> > +    int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t
> *val);
> >
> >      /* Meters */
> >
> >
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 419cb1d..0642cc8 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -67,6 +67,13 @@  enum ct_alg_mode {
     CT_TFTP_MODE,
 };
 
+/* Variable to manage read/write on CT parameters. */
+struct ct_wk_params {
+    char *cli;      /* Parameter name in human format. */
+    int (*wr)(struct conntrack *, uint32_t);
+    int (*rd)(struct conntrack *, uint32_t *);
+};
+
 static bool conn_key_extract(struct conntrack *, struct dp_packet *,
                              ovs_be16 dl_type, struct conn_lookup_ctx *,
                              uint16_t zone);
@@ -2391,6 +2398,66 @@  conntrack_flush(struct conntrack *ct, const uint16_t *zone)
     return 0;
 }
 
+/* List of parameters that can be read/written at run-time. */
+struct ct_wk_params wk_params[] = {};
+
+int
+conntrack_set_param(struct conntrack *ct,
+        const char *set_param)
+{
+    bool valid_param = false;
+    uint32_t max_conn;
+    char bfr[16] = "";
+
+    /* Check if the specified param can be managed. */
+    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
+        if (!strncmp(set_param, wk_params[i].cli,
+                strlen(wk_params[i].cli))) {
+            valid_param = true;
+            ovs_strzcpy(bfr, wk_params[i].cli, sizeof(bfr) - 1);
+            strncat(bfr, "=%"SCNu32, sizeof(bfr) - 1 - strlen(bfr));
+            if (ovs_scan(set_param, bfr, &max_conn)) {
+                return (wk_params[i].wr
+                        ? wk_params[i].wr(ct, max_conn)
+                        : EOPNOTSUPP);
+            } else {
+                return EINVAL;
+            }
+        }
+    }
+    if (!valid_param) {
+        VLOG_DBG("%s: expected valid PARAM=NUMBER", set_param);
+        return EINVAL;
+    }
+
+    return 0;
+}
+
+int
+conntrack_get_param(struct conntrack *ct,
+        const char *get_param, uint32_t *val)
+{
+    bool valid_param = false;
+
+    /* Check if the specified param can be managed. */
+    for (int i = 0; i < sizeof(wk_params) / sizeof(struct ct_wk_params); i++) {
+        if (!strncmp(get_param, wk_params[i].cli,
+                strlen(wk_params[i].cli))) {
+            valid_param = true;
+
+            return (wk_params[i].rd
+                    ? wk_params[i].rd(ct, val)
+                    : EOPNOTSUPP);
+        }
+    }
+    if (!valid_param) {
+        VLOG_DBG("%s: expected a valid PARAM", get_param);
+        return EINVAL;
+    }
+
+    return 0;
+}
+
 /* This function must be called with the ct->resources read lock taken. */
 static struct alg_exp_node *
 expectation_lookup(struct hmap *alg_expectations,
diff --git a/lib/conntrack.h b/lib/conntrack.h
index fbeef1c..4eb9a9a 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -114,6 +114,9 @@  int conntrack_dump_next(struct conntrack_dump *, struct ct_dpif_entry *);
 int conntrack_dump_done(struct conntrack_dump *);
 
 int conntrack_flush(struct conntrack *, const uint16_t *zone);
+int conntrack_set_param(struct conntrack *, const char *set_param);
+int conntrack_get_param(struct conntrack *, const char *get_param,
+                        uint32_t *val);
 
 /* 'struct ct_lock' is a wrapper for an adaptive mutex.  It's useful to try
  * different types of locks (e.g. spinlocks) */
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index c79e69e..599bc57 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -127,6 +127,34 @@  ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
             : EOPNOTSUPP);
 }
 
+int
+ct_dpif_set_param(struct dpif *dpif, const char *set_param)
+{
+    if (!set_param) {
+        VLOG_DBG("%s: ct_set_param: no input param", dpif_name(dpif));
+        return EINVAL;
+    }
+    VLOG_DBG("%s: ct_set_param: %s", dpif_name(dpif), set_param);
+
+    return (dpif->dpif_class->ct_set_param
+            ? dpif->dpif_class->ct_set_param(dpif, set_param)
+            : EOPNOTSUPP);
+}
+
+int
+ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val)
+{
+    if (!get_param) {
+        VLOG_DBG("%s: ct_get_param: no input param", dpif_name(dpif));
+        return EINVAL;
+    }
+    VLOG_DBG("%s: ct_get_param: %s", dpif_name(dpif), get_param);
+
+    return (dpif->dpif_class->ct_get_param
+            ? dpif->dpif_class->ct_get_param(dpif, get_param, val)
+            : EOPNOTSUPP);
+}
+
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index d5f9661..92ce3e9 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -196,6 +196,8 @@  int ct_dpif_dump_start(struct dpif *, struct ct_dpif_dump_state **,
 int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry *);
 int ct_dpif_dump_done(struct ct_dpif_dump_state *);
 int ct_dpif_flush(struct dpif *, const uint16_t *zone);
+int ct_dpif_set_param(struct dpif *dpif, const char *set_param);
+int ct_dpif_get_param(struct dpif *dpif, const char *get_param, uint32_t *val);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
                           bool verbose, bool print_stats);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 8951d6e..2a4a924 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1563,6 +1563,89 @@  dpctl_ct_bkts(int argc, const char *argv[],
     free(conn_per_bkts);
     return error;
 }
+
+static int
+dpctl_ct_set_param(int argc, const char *argv[],
+        struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    char *name;
+    int error;
+
+    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
+    if (!name) {
+        return EINVAL;
+    }
+    error = parsed_dpif_open(name, false, &dpif);
+    free(name);
+    if (error) {
+        dpctl_error(dpctl_p, error, "opening datapath");
+        return error;
+    }
+
+    error = ct_dpif_set_param(dpif, argv[argc - 1]);
+
+    switch (error) {
+    case 0:
+        dpctl_print(dpctl_p, "Ok");
+        break;
+    case EOPNOTSUPP:
+        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
+        break;
+    case EINVAL:
+        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
+        break;
+    default:
+        dpctl_print(dpctl_p, "Error:%d", error);
+        break;
+    }
+    dpif_close(dpif);
+
+    return error;
+}
+
+static int
+dpctl_ct_get_param(int argc, const char *argv[],
+        struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    uint32_t param_val;
+    char *name;
+    int error;
+
+    name = argc == 3 ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
+    if (!name) {
+        return EINVAL;
+    }
+    error = parsed_dpif_open(name, false, &dpif);
+    free(name);
+    if (error) {
+        dpctl_error(dpctl_p, error, "opening datapath");
+        return error;
+    }
+
+    error = ct_dpif_get_param(dpif, argv[argc - 1], &param_val);
+
+    switch (error) {
+    case 0:
+        dpctl_print(dpctl_p, "Current value: %d", param_val);
+        break;
+    case EOPNOTSUPP:
+        dpctl_print(dpctl_p, "Error:%d operation not supported.", error);
+        break;
+    case EINVAL:
+        dpctl_print(dpctl_p, "Error:%d invalid parameter.", error);
+        break;
+    default:
+        dpctl_print(dpctl_p, "Error:%d", error);
+        break;
+    }
+
+    dpif_close(dpif);
+
+    return error;
+}
+
 
 /* Undocumented commands for unit testing. */
 
@@ -1859,6 +1942,8 @@  static const struct dpctl_command all_commands[] = {
     { "ct-stats-show", "[dp] [zone=N] [verbose]",
       0, 3, dpctl_ct_stats_show, DP_RO },
     { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
+    { "ct-set", "[dp] param=..", 1, 2, dpctl_ct_set_param, DP_RW },
+    { "ct-get", "[dp] param", 1, 2, dpctl_ct_get_param, DP_RO },
     { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
     { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index ca74df8..8fda2a9 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5689,6 +5689,23 @@  dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone)
     return conntrack_flush(&dp->conntrack, zone);
 }
 
+static int
+dpif_netdev_ct_set_param(struct dpif *dpif, const char *set_param)
+{
+    struct dp_netdev *dp = get_dp_netdev(dpif);
+
+    return conntrack_set_param(&dp->conntrack, set_param);
+}
+
+static int
+dpif_netdev_ct_get_param(struct dpif *dpif, const char *get_param,
+                         uint32_t *val)
+{
+    struct dp_netdev *dp = get_dp_netdev(dpif);
+
+    return conntrack_get_param(&dp->conntrack, get_param, val);
+}
+
 const struct dpif_class dpif_netdev_class = {
     "netdev",
     dpif_netdev_init,
@@ -5734,6 +5751,8 @@  const struct dpif_class dpif_netdev_class = {
     dpif_netdev_ct_dump_next,
     dpif_netdev_ct_dump_done,
     dpif_netdev_ct_flush,
+    dpif_netdev_ct_set_param,
+    dpif_netdev_ct_get_param,
     dpif_netdev_meter_get_features,
     dpif_netdev_meter_set,
     dpif_netdev_meter_get,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 29001fb..0945fad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2986,6 +2986,8 @@  const struct dpif_class dpif_netlink_class = {
     dpif_netlink_ct_dump_next,
     dpif_netlink_ct_dump_done,
     dpif_netlink_ct_flush,
+    NULL,                       /* ct_set_param */
+    NULL,                       /* ct_get_param */
     dpif_netlink_meter_get_features,
     dpif_netlink_meter_set,
     dpif_netlink_meter_get,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 1d82a09..262b2e0 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -427,6 +427,10 @@  struct dpif_class {
     /* Flushes the connection tracking tables. If 'zone' is not NULL,
      * only deletes connections in '*zone'. */
     int (*ct_flush)(struct dpif *, const uint16_t *zone);
+    /* Set a value to a connection tracking working parameter. */
+    int (*ct_set_param)(struct dpif *, const char *set_param);
+    /* Read the current value of a connection tracking working parameter. */
+    int (*ct_get_param)(struct dpif *, const char *get_param, uint32_t *val);
 
     /* Meters */