Re: [ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
Hi Eelco,
Eelco Chaudron 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
> ---
> 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.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:
(c168f700) 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(, , );
> +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 =
Re: [ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
On 3 Mar 2021, at 21:44, Flavio Leitner wrote: > On Wed, Mar 03, 2021 at 02:53:22PM +0100, Eelco Chaudron wrote: >> 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 >> >> 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 >> --- > > Same nit in the commit with regards to precision as before. > > The API doesn't say anything about cache 'name' pointer ownership > most probably because it is hardcoded to a single name. I think > it's fine for now but in the future we might need to change that > using xstrdup() and let the caller calls free(). > > Thanks for including the test unit. Works here. > > Acked-by: Flavio Leitner Thanks Flavio! Ilya do you need a v4 for the commit message change? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
On Wed, Mar 03, 2021 at 02:53:22PM +0100, Eelco Chaudron wrote: > 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 > > 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 > --- Same nit in the commit with regards to precision as before. The API doesn't say anything about cache 'name' pointer ownership most probably because it is hardcoded to a single name. I think it's fine for now but in the future we might need to change that using xstrdup() and let the caller calls free(). Thanks for including the test unit. Works here. Acked-by: Flavio Leitner ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes
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
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
---
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(+)
diff --git a/datapath/linux/compat/include/linux/openvswitch.h
b/datapath/linux/compat/include/linux/openvswitch.h
index 690d78871..df06d8fd3 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 acc677974..6cba8db51 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -591,6 +591,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, _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, ) ||
+dpif_cache_get_size(dpif, i, )) {
+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)
{
@@ -623,6 +653,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 (_port, , dpif) {
@@ -2409,6 +2441,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, );
+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");
+return error;
+}
+
+if (!ovs_scan(argv[3], "%"SCNu32, )) {
+dpctl_error(dpctl_p, error, "size is malformed");
+return error;
+}
+
+error = parsed_dpif_open(argv[1], false, );
+if (error) {
+dpctl_error(dpctl_p, error, "Opening datapath %s failed",
+argv[1]);
+
