[ovs-dev] [PATCH 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes

Flavio Leitner fbl at sysclose.org
Mon Mar 1 21:04:51 UTC 2021


On Wed, Sep 02, 2020 at 12:12:44PM +0200, Eelco Chaudron wrote:
> This patch adds a general way of viewing/configuring datapath
> cache sizes. With an implementation for the netlink interface.

I see you added support to multiple cache levels. Does that
mean we can use to set sizes of EMC and SMC?

> The ovs-dpctl/ovs-appctl show commands will display the
> current cache sizes configured:
> 
> ovs-dpctl show
> system at ovs-system:
>   lookups: hit:25 missed:63 lost:0
>   flows: 0
>   masks: hit:282 total:0 hit/pkt:3.20
>   cache: hit:4 hit rate:4.5455%
>   caches:
>     masks cache: size: 256
>   port 0: ovs-system (internal)
>   port 1: br-int (internal)
>   port 2: genev_sys_6081 (geneve: packet_type=ptap)
>   port 3: br-ex (internal)
>   port 4: eth2
>   port 5: sw0p1 (internal)
>   port 6: sw0p3 (internal)
> 
> A specific cache can be configured as follows:
> 
> ovs-appctl dpctl/cache-set-size DP CACHE SIZE
> ovs-dpctl cache-set-size DP CACHE SIZE
> 
> For example to disable the cache do:
> 
> $ ovs-dpctl cache-set-size system at ovs-system "masks cache" 0

Should we use cache names with spaces?


> Setting cache size successful, new size 0.
> 
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
> ---
>  datapath/linux/compat/include/linux/openvswitch.h |    1 
>  lib/dpctl.c                                       |  120 +++++++++++++++++++++
>  lib/dpif-netdev.c                                 |    4 +
>  lib/dpif-netlink.c                                |  113 ++++++++++++++++++++
>  lib/dpif-provider.h                               |   20 ++++
>  lib/dpif.c                                        |   32 ++++++
>  lib/dpif.h                                        |    7 +
>  utilities/ovs-dpctl.c                             |    4 +
>  8 files changed, 301 insertions(+)
> 
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index fb73bfa90..4d4ead8af 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -105,6 +105,7 @@ enum ovs_datapath_attr {
>  	OVS_DP_ATTR_MEGAFLOW_STATS,	/* struct ovs_dp_megaflow_stats */
>  	OVS_DP_ATTR_USER_FEATURES,	/* OVS_DP_F_*  */
>  	OVS_DP_ATTR_PAD,
> +	OVS_DP_ATTR_MASKS_CACHE_SIZE,
>  	__OVS_DP_ATTR_MAX
>  };
>  
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index dac350fb5..c8e8b3cdb 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -585,6 +585,36 @@ compare_port_nos(const void *a_, const void *b_)
>      return a < b ? -1 : a > b;
>  }
>  
> +static void
> +show_dpif_cache__(struct dpif *dpif, struct dpctl_params *dpctl_p)
> +{
> +    uint32_t nr_caches;
> +    int error = dpif_cache_get_supported_levels(dpif, &nr_caches);
> +
> +    if (error || nr_caches == 0) {
> +        return;
> +    }
> +
> +    dpctl_print(dpctl_p, "  caches:\n");
> +    for (int i = 0; i < nr_caches; i++) {
> +        const char *name;
> +        uint32_t size;
> +
> +        if (dpif_cache_get_name(dpif, i, &name) ||
> +            dpif_cache_get_size(dpif, i, &size)) {
> +            continue;
> +        }
> +        dpctl_print(dpctl_p, "    %s: size: %u\n", name, size);
> +    }
> +}
> +
> +static void
> +show_dpif_cache(struct dpif *dpif, struct dpctl_params *dpctl_p)
> +{
> +    dpctl_print(dpctl_p, "%s:\n", dpif_name(dpif));
> +    show_dpif_cache__(dpif, dpctl_p);
> +}
> +
>  static void
>  show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
>  {
> @@ -617,6 +647,8 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
>          }
>      }
>  
> +    show_dpif_cache__(dpif, dpctl_p);
> +
>      odp_port_t *port_nos = NULL;
>      size_t allocated_port_nos = 0, n_port_nos = 0;
>      DPIF_PORT_FOR_EACH (&dpif_port, &dump, dpif) {
> @@ -2224,6 +2256,92 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
>      return error;
>  }
>  
> +static int
> +dpctl_cache_get_size(int argc, const char *argv[],
> +                     struct dpctl_params *dpctl_p)
> +{
> +    int error;
> +
> +    if (argc > 1) {
> +        struct dpif *dpif;
> +
> +        error = parsed_dpif_open(argv[1], false, &dpif);
> +        if (!error) {
> +            show_dpif_cache(dpif, dpctl_p);
> +            dpif_close(dpif);
> +        } else {
> +            dpctl_error(dpctl_p, error, "opening datapath %s failed", argv[1]);
> +        }
> +    } else {
> +        error = dps_for_each(dpctl_p, show_dpif_cache);
> +    }
> +
> +    return error;
> +}
> +
> +static int
> +dpctl_cache_set_size(int argc, const char *argv[],
> +                     struct dpctl_params *dpctl_p)
> +{
> +    int i, error = EINVAL;
> +    uint32_t nr_caches, size;
> +    struct dpif *dpif;
> +
> +    if (argc != 4) {
> +        dpctl_error(dpctl_p, error, "Invalid number of arguments passes.");

s/passes// ?

> +        return error;
> +    }
> +
> +    if (!ovs_scan(argv[3], "%"SCNu32, &size)) {
> +        dpctl_error(dpctl_p, error, "size seems malformed.");

s/seems/is/ ?

> +        return error;
> +    }
> +
> +    error = parsed_dpif_open(argv[1], false, &dpif);
> +    if (error) {
> +            dpctl_error(dpctl_p, error, "Opening datapath %s failed.",
> +                        argv[1]);
> +            return error;
> +    }
> +
> +    error = dpif_cache_get_supported_levels(dpif, &nr_caches);
> +    if (error || nr_caches == 0) {
> +        dpctl_error(dpctl_p, error, "Setting caches not supported.");
> +        goto exit;
> +    }
> +
> +    for (i = 0; i < nr_caches; i++) {
> +        const char *name;
> +
> +        if (dpif_cache_get_name(dpif, i, &name)) {
> +            continue;
> +        }
> +        if (!strcmp(argv[2], name)) {

Shouldn't it use strncmp?

> +            break;
> +        }
> +    }
> +
> +    if (i == nr_caches) {
> +        dpctl_error(dpctl_p, error, "Caches name \"%s\" no found on dpif.",
> +                    argv[2]);

s/Caches/Cache/ ?

> +        error = EINVAL;
> +        goto exit;
> +    }
> +
> +    error = dpif_cache_set_size(dpif, i, size);
> +    if (!error) {
> +        dpif_cache_get_size(dpif, i, &size);
> +        dpctl_print(dpctl_p, "Setting cache size successful, new size %u.\n",
> +                    size);
> +    } else {
> +        dpctl_error(dpctl_p, error, "Setting cache size failed!");
> +    }
> +
> +exit:
> +    dpif_close(dpif);
> +    return error;
> +}
> +
>  /* Undocumented commands for unit testing. */
>  
>  static int
> @@ -2522,6 +2640,8 @@ static const struct dpctl_command all_commands[] = {
>      { "dump-conntrack", "[dp] [zone=N]", 0, 2, dpctl_dump_conntrack, DP_RO },
>      { "flush-conntrack", "[dp] [zone=N] [ct-tuple]", 0, 3,
>        dpctl_flush_conntrack, DP_RW },
> +    { "cache-get-size", "[dp]", 0, 1, dpctl_cache_get_size, DP_RO },
> +    { "cache-set-size", "dp cache <size>", 3, 3, dpctl_cache_set_size, DP_RW },


Do you mind to update lib/dpctl.man as well ?


>      { "ct-stats-show", "[dp] [zone=N]",
>        0, 3, dpctl_ct_stats_show, DP_RO },
>      { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, DP_RO },
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4e3bd7519..8a14c71aa 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8465,6 +8465,10 @@ const struct dpif_class dpif_netdev_class = {
>      dpif_netdev_bond_add,
>      dpif_netdev_bond_del,
>      dpif_netdev_bond_stats_get,
> +    NULL,                       /* cache_get_supported_levels */
> +    NULL,                       /* cache_get_name */
> +    NULL,                       /* cache_get_size */
> +    NULL,                       /* cache_set_size */
>  };
>  
>  static void
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 07f15ac65..dcc1d5e57 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -95,6 +95,7 @@ struct dpif_netlink_dp {
>      const char *name;                  /* OVS_DP_ATTR_NAME. */
>      const uint32_t *upcall_pid;        /* OVS_DP_ATTR_UPCALL_PID. */
>      uint32_t user_features;            /* OVS_DP_ATTR_USER_FEATURES */
> +    uint32_t cache_size;               /* OVS_DP_ATTR_MASKS_CACHE_SIZE */
>      const struct ovs_dp_stats *stats;  /* OVS_DP_ATTR_STATS. */
>      const struct ovs_dp_megaflow_stats *megaflow_stats;
>                                         /* OVS_DP_ATTR_MEGAFLOW_STATS.*/
> @@ -3983,6 +3984,100 @@ probe_broken_meters(struct dpif *dpif)
>      }
>      return broken_meters;
>  }
> +
> +
> +static int
> +dpif_netlink_cache_get_supported_levels(struct dpif *dpif_, uint32_t *levels)
> +{
> +    int error;
> +    struct ofpbuf *buf;
> +    struct dpif_netlink_dp dp;
> +
> +    /* If available, in the kernel we support one level of cache.
> +     * Unfortunately, there is no way to detect if the older kernel module has
> +     * the cache feature. For now, we only report the cache information if the
> +     * kernel module reports the  OVS_DP_ATTR_MASKS_CACHE_SIZE attribute. */
> +
> +    *levels = 0;
> +    error = dpif_netlink_dp_get(dpif_, &dp, &buf);
> +    if (!error) {
> +
> +        if (dp.cache_size != UINT32_MAX) {
> +            *levels = 1;
> +        }
> +        ofpbuf_delete(buf);
> +    }
> +
> +    return error;
> +}
> +
> +static int
> +dpif_netlink_cache_get_name(struct dpif *dpif_ OVS_UNUSED, uint32_t level,
> +                            const char **name)
> +{
> +    if (level != 0) {
> +        return EINVAL;
> +    }
> +
> +    *name = "masks cache";

As I mentioned before, maybe we should use "masks-cache" ?
Just a suggestion.


> +    return 0;
> +}
> +
> +static int
> +dpif_netlink_cache_get_size(struct dpif *dpif_, uint32_t level, uint32_t *size)
> +{
> +    int error;
> +    struct ofpbuf *buf;
> +    struct dpif_netlink_dp dp;
> +
> +    if (level != 0) {
> +        return EINVAL;
> +    }
> +
> +    error = dpif_netlink_dp_get(dpif_, &dp, &buf);
> +    if (!error) {
> +
> +        ofpbuf_delete(buf);
> +
> +        if (dp.cache_size == UINT32_MAX) {
> +            return EOPNOTSUPP;
> +        }
> +        *size = dp.cache_size;
> +    }
> +    return error;
> +}
> +
> +static int
> +dpif_netlink_cache_set_size(struct dpif *dpif_, uint32_t level, uint32_t size)
> +{
> +    int error;
> +    struct ofpbuf *bufp;
> +    struct dpif_netlink_dp request, reply;
> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> +
> +    size = ROUND_UP_POW2(size);
> +
> +    if (level != 0) {
> +        return EINVAL;
> +    }
> +
> +    dpif_netlink_dp_init(&request);
> +    request.cmd = OVS_DP_CMD_SET;
> +    request.name = dpif_->base_name;
> +    request.dp_ifindex = dpif->dp_ifindex;
> +    request.cache_size = size;
> +
> +    error = dpif_netlink_dp_transact(&request, &reply, &bufp);
> +    if (!error) {
> +        ofpbuf_delete(bufp);
> +        if (reply.cache_size != size) {
> +            return EINVAL;
> +        }
> +    }
> +
> +    return error;
> +}
> +
>  
>  const struct dpif_class dpif_netlink_class = {
>      "system",
> @@ -4060,6 +4155,10 @@ const struct dpif_class dpif_netlink_class = {
>      NULL,                       /* bond_add */
>      NULL,                       /* bond_del */
>      NULL,                       /* bond_stats_get */
> +    dpif_netlink_cache_get_supported_levels,
> +    dpif_netlink_cache_get_name,
> +    dpif_netlink_cache_get_size,
> +    dpif_netlink_cache_set_size,
>  };
>  
>  static int
> @@ -4320,6 +4419,9 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp *dp, const struct ofpbuf *buf
>          [OVS_DP_ATTR_USER_FEATURES] = {
>                          .type = NL_A_U32,
>                          .optional = true },
> +        [OVS_DP_ATTR_MASKS_CACHE_SIZE] = {
> +                        .type = NL_A_U32,
> +                        .optional = true },
>      };
>  
>      dpif_netlink_dp_init(dp);
> @@ -4352,6 +4454,12 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp *dp, const struct ofpbuf *buf
>          dp->user_features = nl_attr_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
>      }
>  
> +    if (a[OVS_DP_ATTR_MASKS_CACHE_SIZE]) {
> +        dp->cache_size = nl_attr_get_u32(a[OVS_DP_ATTR_MASKS_CACHE_SIZE]);
> +    } else {
> +        dp->cache_size = UINT32_MAX;
> +    }
> +
>      return 0;
>  }
>  
> @@ -4380,6 +4488,10 @@ dpif_netlink_dp_to_ofpbuf(const struct dpif_netlink_dp *dp, struct ofpbuf *buf)
>          nl_msg_put_u32(buf, OVS_DP_ATTR_USER_FEATURES, dp->user_features);
>      }
>  
> +    if (dp->cache_size != UINT32_MAX) {
> +        nl_msg_put_u32(buf, OVS_DP_ATTR_MASKS_CACHE_SIZE, dp->cache_size);
> +    }
> +
>      /* Skip OVS_DP_ATTR_STATS since we never have a reason to serialize it. */
>  }
>  
> @@ -4388,6 +4500,7 @@ static void
>  dpif_netlink_dp_init(struct dpif_netlink_dp *dp)
>  {
>      memset(dp, 0, sizeof *dp);
> +    dp->cache_size = UINT32_MAX;
>  }
>  
>  static void
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 0e024c1c9..61bd90f57 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -628,6 +628,26 @@ struct dpif_class {
>       * sufficient to store BOND_BUCKETS number of elements. */
>      int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
>                            uint64_t *n_bytes);
> +
> +    /* Cache configuration
> +     *
> +     * Multiple levels of cache can exist in a given datapath implementation.
> +     * An API has been provided to get the number of supported caches, which
> +     * can then be used to get/set specific configuration. Cache level is 0
> +     * indexed, i.e. if 1 level is supported, the level value to use is 0.
> +     *
> +     * Get the number of cache levels supported. */
> +    int (*cache_get_supported_levels)(struct dpif *dpif, uint32_t *levels);
> +
> +    /* Get the cache name for the given level. */
> +    int (*cache_get_name)(struct dpif *dpif, uint32_t level,
> +                          const char **name);
> +
> +    /* Get currently configured cache size. */
> +    int (*cache_get_size)(struct dpif *dpif, uint32_t level, uint32_t *size);
> +
> +    /* Set cache size. */
> +    int (*cache_set_size)(struct dpif *dpif, uint32_t level, uint32_t size);
>  };
>  
>  extern const struct dpif_class dpif_netlink_class;
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 7cac3a629..4db897c7d 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -2018,3 +2018,35 @@ dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id,
>             ? dpif->dpif_class->bond_stats_get(dpif, bond_id, n_bytes)
>             : EOPNOTSUPP;
>  }
> +
> +int
> +dpif_cache_get_supported_levels(struct dpif *dpif, uint32_t *levels)
> +{
> +    return dpif->dpif_class->cache_get_supported_levels
> +        ? dpif->dpif_class->cache_get_supported_levels(dpif, levels)
> +        : EOPNOTSUPP;
> +}
> +
> +int
> +dpif_cache_get_name(struct dpif *dpif, uint32_t level, const char **name)
> +{
> +    return dpif->dpif_class->cache_get_name
> +        ? dpif->dpif_class->cache_get_name(dpif, level, name)
> +        : EOPNOTSUPP;
> +}
> +
> +int
> +dpif_cache_get_size(struct dpif *dpif, uint32_t level, uint32_t *size)
> +{
> +    return dpif->dpif_class->cache_get_size
> +        ? dpif->dpif_class->cache_get_size(dpif, level, size)
> +        : EOPNOTSUPP;
> +}
> +
> +int
> +dpif_cache_set_size(struct dpif *dpif, uint32_t level, uint32_t size)
> +{
> +    return dpif->dpif_class->cache_set_size
> +        ? dpif->dpif_class->cache_set_size(dpif, level, size)
> +        : EOPNOTSUPP;
> +}
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 43c1ab998..35f48bec6 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -905,6 +905,13 @@ int dpif_bond_del(struct dpif *, uint32_t bond_id);
>  int dpif_bond_stats_get(struct dpif *, uint32_t bond_id, uint64_t *n_bytes);
>  bool dpif_supports_lb_output_action(const struct dpif *);
>  
> +
> +/* Cache */
> +int dpif_cache_get_supported_levels(struct dpif *dpif, uint32_t *levels);
> +int dpif_cache_get_name(struct dpif *dpif, uint32_t level, const char **name);
> +int dpif_cache_get_size(struct dpif *dpif, uint32_t level, uint32_t *size);
> +int dpif_cache_set_size(struct dpif *dpif, uint32_t level, uint32_t size);
> +
>  
>  /* Miscellaneous. */
>  
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index 7d99607f4..66055cfe2 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -195,6 +195,10 @@ usage(void *userdata OVS_UNUSED)
>             "  get-flow [DP] ufid:UFID    fetch flow corresponding to UFID\n"
>             "  del-flow [DP] FLOW         delete FLOW from DP\n"
>             "  del-flows [DP]             delete all flows from DP\n"
> +           "  cache-get-size [DP]             " \
> +               "Show the current size for all caches\n"
> +           "  cache-set-size DP CACHE SIZE  " \
> +               "Set cache size for a specific cache\n"
>             "  dump-conntrack [DP] [zone=ZONE]  " \
>                 "display conntrack entries for ZONE\n"
>             "  flush-conntrack [DP] [zone=ZONE] [ct-tuple]" \



Do you think it is possible to include a check-kernel test unit?
I.e. if the running version is above the one with the introduced
cache size feature, try to set and get it back otherwise skip?

Thanks,
fbl


More information about the dev mailing list