[ovs-dev] [PATCH v2] dpif-netlink: Fix issues of the offloaded flows counter
Jianbo Liu
jianbol at mellanox.com
Tue Dec 15 06:30:16 UTC 2020
The 12/15/2020 04:50, Jianbo Liu wrote:
> The n_offloaded_flows counter is saved in dpif, and this is the first
> one when ofproto is created. When flow operation is done by ovs-appctl
> commands, such as, dpctl/add-flow, a new dpif is opened, and the
> n_offloaded_flows in it can't be used. So, instead of using counter,
> the number of offloaded flows is queried from each netdev, then sum
> them up. To achieve this, a new API is added in netdev_flow_api to get
> how many flows assigned to a netdev.
>
> In order to get better performance, this number is calculated directly
> from tc_to_ufid hmap for netdev-offload-tc, because flow dumping by tc
> takes much time if there are many flows offloaded.
>
> Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules")
> Signed-off-by: Jianbo Liu <jianbol at nvidia.com>
> ---
> lib/dpif-netlink.c | 9 ---------
> lib/dpif.c | 22 ++++++++++++++++++++++
> lib/dpif.h | 3 ++-
> lib/netdev-offload-provider.h | 4 ++++
> lib/netdev-offload-tc.c | 20 ++++++++++++++++++++
> lib/netdev-offload.c | 27 +++++++++++++++++++++++++++
> lib/netdev-offload.h | 3 +++
> ofproto/ofproto-dpif-upcall.c | 10 ++++------
> tests/system-offloads-traffic.at | 4 ++++
> 9 files changed, 86 insertions(+), 16 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 6858ba612..2f881e4fa 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -208,7 +208,6 @@ struct dpif_netlink {
> /* Change notification. */
> struct nl_sock *port_notifier; /* vport multicast group subscriber. */
> bool refresh_channels;
> - struct atomic_count n_offloaded_flows;
> };
>
> static void report_loss(struct dpif_netlink *, struct dpif_channel *,
> @@ -654,7 +653,6 @@ dpif_netlink_run(struct dpif *dpif_)
> static int
> dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
> {
> - struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> struct dpif_netlink_dp dp;
> struct ofpbuf *buf;
> int error;
> @@ -680,7 +678,6 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
> }
> ofpbuf_delete(buf);
> }
> - stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows);
> return error;
> }
>
> @@ -2192,9 +2189,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
> }
>
> err = parse_flow_put(dpif, put);
> - if (!err && (put->flags & DPIF_FP_CREATE)) {
> - atomic_count_inc(&dpif->n_offloaded_flows);
> - }
> log_flow_put_message(&dpif->dpif, &this_module, put, 0);
> break;
> }
> @@ -2209,9 +2203,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
> dpif_normalize_type(dpif_type(&dpif->dpif)),
> del->ufid,
> del->stats);
> - if (!err) {
> - atomic_count_dec(&dpif->n_offloaded_flows);
> - }
> log_flow_del_message(&dpif->dpif, &this_module, del, 0);
> break;
> }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index ac2860764..7f01aa48b 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -2018,3 +2018,25 @@ 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_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows)
> +{
> + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
> + struct dpif_port_dump port_dump;
> + struct dpif_port dpif_port;
> + int ret, n_devs = 0;
> + uint64_t nflows;
> +
> + *n_flows = 0;
> + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
> + ret = netdev_ports_get_n_flows(dpif_type_str, &dpif_port, &nflows);
> + if (!ret) {
> + *n_flows += nflows;
> + } else if (ret == EOPNOTSUPP) {
> + continue;
> + }
> + n_devs++;
> + }
> + return n_devs ? 0 : EOPNOTSUPP;
> +}
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 7ef148c6d..9d0d662fc 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -429,7 +429,6 @@ struct dpif_dp_stats {
> uint64_t n_missed; /* Number of flow table misses. */
> uint64_t n_lost; /* Number of misses not sent to userspace. */
> uint64_t n_flows; /* Number of flows present. */
> - uint64_t n_offloaded_flows; /* Number of offloaded flows present. */
> uint64_t n_mask_hit; /* Number of mega flow masks visited for
> flow table matches. */
> uint32_t n_masks; /* Number of mega flow masks. */
> @@ -438,6 +437,8 @@ int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *);
>
> int dpif_set_features(struct dpif *, uint32_t new_features);
>
> +int dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows);
> +
>
> /* Port operations. */
>
> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
> index 0bed7bf61..cf859d1b4 100644
> --- a/lib/netdev-offload-provider.h
> +++ b/lib/netdev-offload-provider.h
> @@ -83,6 +83,10 @@ struct netdev_flow_api {
> int (*flow_del)(struct netdev *, const ovs_u128 *ufid,
> struct dpif_flow_stats *);
>
> + /* Get the number of flows offloaded to netdev.
> + * Return 0 if successful, otherwise returns a positive errno value. */
> + int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows);
> +
> /* Initializies the netdev flow api.
> * Return 0 if successful, otherwise returns a positive errno value. */
> int (*init_flow_api)(struct netdev *);
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 2a772a971..f17980a47 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1904,6 +1904,25 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> return error;
> }
>
> +static int
> +netdev_tc_get_n_flows(struct netdev *netdev, uint64_t *n_flows)
> +{
> + struct ufid_tc_data *data, *next;
> + uint64_t total = 0;
> +
> + ovs_mutex_lock(&ufid_lock);
> + HMAP_FOR_EACH_SAFE (data, next, tc_to_ufid_node, &tc_to_ufid) {
> + if (data->netdev != netdev) {
> + continue;
> + }
> + total++;
> + }
> + ovs_mutex_unlock(&ufid_lock);
> +
> + *n_flows = total;
> + return 0;
> +}
> +
> static void
> probe_multi_mask_per_prio(int ifindex)
> {
> @@ -2044,5 +2063,6 @@ const struct netdev_flow_api netdev_offload_tc = {
> .flow_put = netdev_tc_flow_put,
> .flow_get = netdev_tc_flow_get,
> .flow_del = netdev_tc_flow_del,
> + .flow_get_n_flows = netdev_tc_get_n_flows,
> .init_flow_api = netdev_tc_init_flow_api,
> };
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 2da3bc701..696877058 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -280,6 +280,17 @@ netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
> : EOPNOTSUPP;
> }
>
> +int
> +netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows)
> +{
> + const struct netdev_flow_api *flow_api =
> + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api);
> +
> + return (flow_api && flow_api->flow_get_n_flows)
> + ? flow_api->flow_get_n_flows(netdev, n_flows)
> + : EOPNOTSUPP;
> +}
> +
> int
> netdev_init_flow_api(struct netdev *netdev)
> {
> @@ -602,6 +613,22 @@ netdev_ports_remove(odp_port_t port_no, const char *dpif_type)
> return ret;
> }
>
> +int
> +netdev_ports_get_n_flows(const char *dpif_type, struct dpif_port *dpif_port,
> + uint64_t *n_flows)
> +{
> + struct port_to_netdev_data *data;
> + int ret = EOPNOTSUPP;
> +
> + ovs_rwlock_rdlock(&netdev_hmap_rwlock);
> + data = netdev_ports_lookup(dpif_port->port_no, dpif_type);
> + if (data) {
> + ret = netdev_flow_get_n_flows(data->netdev, n_flows);
> + }
> + ovs_rwlock_unlock(&netdev_hmap_rwlock);
> + return ret;
> +}
> +
> odp_port_t
> netdev_ifindex_to_odp_port(int ifindex)
> {
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 4c0ed2ae8..ffd321bfc 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -103,6 +103,7 @@ bool netdev_any_oor(void);
> bool netdev_is_flow_api_enabled(void);
> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
> bool netdev_is_offload_rebalance_policy_enabled(void);
> +int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows);
>
> struct dpif_port;
> int netdev_ports_insert(struct netdev *, const char *dpif_type,
> @@ -124,6 +125,8 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match,
> struct dpif_flow_stats *stats,
> struct dpif_flow_attrs *attrs,
> struct ofpbuf *buf);
> +int netdev_ports_get_n_flows(const char *dpif_type,
> + struct dpif_port *dpif_port, uint64_t *n_flows);
>
> #ifdef __cplusplus
> }
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 19b92dfe0..75496ac6d 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -175,7 +175,6 @@ struct udpif {
>
> /* n_flows_mutex prevents multiple threads updating these concurrently. */
> atomic_uint n_flows; /* Number of flows in the datapath. */
> - atomic_uint n_offloaded_flows; /* Number of the offloaded flows. */
> atomic_llong n_flows_timestamp; /* Last time n_flows was updated. */
> struct ovs_mutex n_flows_mutex;
>
> @@ -731,8 +730,6 @@ udpif_get_n_flows(struct udpif *udpif)
> dpif_get_dp_stats(udpif->dpif, &stats);
> flow_count = stats.n_flows;
> atomic_store_relaxed(&udpif->n_flows, flow_count);
> - atomic_store_relaxed(&udpif->n_offloaded_flows,
> - stats.n_offloaded_flows);
> ovs_mutex_unlock(&udpif->n_flows_mutex);
> } else {
> atomic_read_relaxed(&udpif->n_flows, &flow_count);
> @@ -2904,10 +2901,10 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> {
> struct ds ds = DS_EMPTY_INITIALIZER;
> + uint64_t n_offloaded_flows;
> struct udpif *udpif;
>
> LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
> - unsigned int n_offloaded_flows;
> unsigned int flow_limit;
> bool ufid_enabled;
> size_t i;
> @@ -2919,8 +2916,9 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> ds_put_format(&ds, " flows : (current %lu)"
> " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
> udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
> - atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows);
> - ds_put_format(&ds, " offloaded flows : %u\n", n_offloaded_flows);
> + if (!dpif_get_dp_offloaded_flows(udpif->dpif, &n_offloaded_flows)) {
> + ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows);
> + }
> ds_put_format(&ds, " dump duration : %lldms\n", udpif->dump_duration);
> ds_put_format(&ds, " ufid enabled : ");
> if (ufid_enabled) {
> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> index 379a8a5e9..c03ffd598 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -32,6 +32,8 @@ in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:882, used:
>
> AT_CHECK([ovs-appctl dpctl/dump-flows type=offloaded], [0], [])
>
> +AT_CHECK([ovs-appctl upcall/show | grep "offloaded flows : 0"], [1], [ignore])
With this change, "offloaded flows" will not be displayed if hw offload
is disabled.
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> @@ -64,5 +66,7 @@ in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:
> in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:0.001s, actions:output
> ])
>
> +AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [ignore])
It's 8 offloaded rules, but I got another value sometimes, which was
caused by testing environment. So I don't check the exact value here.
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> --
> 2.26.2
>
--
More information about the dev
mailing list