[ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
Paolo Valerio
pvalerio at redhat.com
Mon May 24 16:19:29 UTC 2021
Hi Eelco,
Eelco Chaudron <echaudro at redhat.com> writes:
> This patch adds a general way of viewing/configuring datapath
> cache sizes. With an implementation for the netlink interface.
>
> 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
>
there's an implication here based on the current dp code (see below)
> For example to disable the cache do:
>
> $ ovs-dpctl cache-set-size system at ovs-system masks-cache 0
> Setting cache size successful, new size 0.
>
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
> ---
> v2: - Changed cache naming to use a hyphen instead of spaces
> - Some error message grammar changes
> - Update dpctl man page
> - Add self tests to for the new set/get commands
> v3: - Rebase on the latest master branch
>
> datapath/linux/compat/include/linux/openvswitch.h | 1
> lib/dpctl.c | 120 +++++++++++++++++++++
> lib/dpctl.man | 14 ++
> lib/dpif-netdev.c | 4 +
> lib/dpif-netlink.c | 113 ++++++++++++++++++++
> lib/dpif-provider.h | 20 ++++
> lib/dpif.c | 32 ++++++
> lib/dpif.h | 7 +
> tests/system-traffic.at | 36 ++++++
> utilities/ovs-dpctl.c | 4 +
> 10 files changed, 351 insertions(+)
>
[...]
> +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;
> +
with this nl transaction, we lose the 'user_features' because it gets
blanked in the datapath that, ATM, always requires user_featues to be
present.
if we issue:
# ovs-appctl dpctl/cache-set-size system at ovs-system masks-cache 512
probing user_features value (previously set to 0x7):
ovs-vswitchd 44589 [010] 12160.797672: probe:ovs_dp_change_L30: (ffffffffc168f700) user_features=0x0
Note that a new dp open will set back the correct value.
We have two potential solutions here:
the first one is in userspace, and requires, for newer userspace
versions, that OVS_DP_ATTR_USER_FEATURES becomes mandatory (this has to
be always included from userspace unless it's not supported).
In this case we should just add:
request.user_features = dpif->user_features;
The alternative involves changing the kernel's behavior.
I tested both approaches, and both do the job.
Based on the preference we could modify this patch, or update the
kernel's behavior.
Paolo
> + 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",
> @@ -4026,6 +4121,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
> @@ -4286,6 +4385,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);
> @@ -4318,6 +4420,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;
> }
>
> @@ -4346,6 +4454,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. */
> }
>
> @@ -4354,6 +4466,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 b817fceac..506485258 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 56d0b4a65..334e7707f 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -2041,3 +2041,35 @@ dpif_get_n_offloaded_flows(struct dpif *dpif, uint64_t *n_flows)
> }
> return n_devs ? 0 : 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 600e4522c..92ea30c1b 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -907,6 +907,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/tests/system-traffic.at b/tests/system-traffic.at
> index fb5b9a36d..e4fa8da7d 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1400,6 +1400,42 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([datapath - configure cache size])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_KERNEL_EXCL(3, 10, 5, 8)
> +
> +AT_CHECK([ovs-dpctl cache-get-size one-bad-dp], [1], [], [dnl
> +ovs-dpctl: Opening datapath one-bad-dp failed (No such device)
> +])
> +AT_CHECK([ovs-dpctl cache-get-size | grep masks-cache | tr -d [[:blank:]]], [0], [dnl
> +masks-cache:size:256
> +])
> +AT_CHECK([ovs-dpctl cache-set-size one-bad-dp masks-cache 0], [1], [], [dnl
> +ovs-dpctl: Opening datapath one-bad-dp failed (No such device)
> +])
> +AT_CHECK([ovs-dpctl cache-set-size system at ovs-system dummy-cache 0], [1], [], [dnl
> +ovs-dpctl: Cache name "dummy-cache" not found on dpif (Invalid argument)
> +])
> +AT_CHECK([ovs-dpctl cache-set-size system at ovs-system masks-cache 80000], [1], [], [dnl
> +ovs-dpctl: Setting cache size failed (Numerical result out of range)
> +])
> +AT_CHECK([ovs-dpctl cache-set-size system at ovs-system masks-cache 0], [0], [dnl
> +Setting cache size successful, new size 0
> +])
> +AT_CHECK([ovs-dpctl cache-get-size | grep masks-cache | tr -d [[:blank:]]], [0], [dnl
> +masks-cache:size:0
> +])
> +AT_CHECK([ovs-dpctl cache-set-size system at ovs-system masks-cache 256], [0], [dnl
> +Setting cache size successful, new size 256
> +])
> +AT_CHECK([ovs-dpctl cache-get-size | grep masks-cache | tr -d [[:blank:]]], [0], [dnl
> +masks-cache:size:256
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_BANNER([conntrack])
>
> AT_SETUP([conntrack - controller])
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index f616995c3..56d7a942b 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -198,6 +198,10 @@ usage(void *userdata OVS_UNUSED)
> " del-flow [DP] FLOW delete FLOW from DP\n"
> " del-flows [DP] [FILE] " \
> "delete all or specified 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]" \
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list