Hi Eelco, Eelco Chaudron <echau...@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@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@ovs-system masks-cache 0 > Setting cache size successful, new size 0. > > Signed-off-by: Eelco Chaudron <echau...@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@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@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@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@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@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev