[ovs-dev] [PATCH 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
Eelco Chaudron
echaudro at redhat.com
Wed Mar 3 10:09:05 UTC 2021
On 1 Mar 2021, at 22:04, Flavio Leitner wrote:
> 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?
Yes, the idea was that the same API can be used for the userspace
EMC/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?
I thought that with a space, it looked more aesthetic, however, it's
more cumbersome to use, i.e. it needs quotes. So I changed it to use a
hyphen in the v2, which I will send out soon.
>> 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// ?
Will fix in V2
>> + return error;
>> + }
>> +
>> + if (!ovs_scan(argv[3], "%"SCNu32, &size)) {
>> + dpctl_error(dpctl_p, error, "size seems malformed.");
>
> s/seems/is/ ?
Will fix in V2
>> + 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?
It needs a full match, so no need for strncmp.
>> + break;
>> + }
>> + }
>> +
>> + if (i == nr_caches) {
>> + dpctl_error(dpctl_p, error, "Caches name \"%s\" no found on
>> dpif.",
>> + argv[2]);
>
> s/Caches/Cache/ ?
Will fix in V2
>> + 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 ?
Will add in V2
>> { "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.
Done!
>> + 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?
Done in v2
> Thanks,
> fbl
More information about the dev
mailing list