On 16.12.2019 16:10, Eli Britstein wrote: > From: Ophir Munk <ophi...@mellanox.com> > > In case a flow is HW offloaded, packets do not reach the SW, thus not > counted for statistics. Use netdev flow get API in order to update the > statistics of flows by the HW statistics. > > Co-authored-by: Eli Britstein <el...@mellanox.com> > Signed-off-by: Ophir Munk <ophi...@mellanox.com> > Reviewed-by: Oz Shlomo <o...@mellanox.com> > Signed-off-by: Eli Britstein <el...@mellanox.com> > --- > lib/dpif-netdev.c | 81 > ++++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 68 insertions(+), 13 deletions(-)
Some issues of this patch: 1. stats are duplicated now in dpif-netdev. We already have them in netdev-offload-dpdk, so we don't need to store them here too. 2. dpif_flow_attrs should be filled by the offload provider, not by the datapath. netdev-offload-tc fills this information and it will be lost in current implementation of dpif-netdev. 3. New 'dp_layer' types has to be documented in dpctl man. Also, 'in_hw' doesn't look like a datapath layer name. Suggesting to use 'dpdk' as netdev-offload-tc uses 'tc'. We also could return specific dp_layer for partially offloaded flows, i.e. 'ovs-partial'. Suggesting following incremental patch where I tried to address above issues (didn't update man, only compile tested, made on top of the full set): ----- Subject: [PATCH] dpif-netdev: Cleanup for stats/attrs of offloaded flows. Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> --- lib/dpif-netdev.c | 146 +++++++++++++++++++------------------- lib/netdev-offload-dpdk.c | 35 +++++---- 2 files changed, 94 insertions(+), 87 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 91fefb877..892b2d0f5 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -530,7 +530,6 @@ struct dp_netdev_flow { /* Statistics. */ struct dp_netdev_flow_stats stats; - struct dp_netdev_flow_stats hw_stats; /* Actions. */ OVSRCU_TYPE(struct dp_netdev_actions *) actions; @@ -3011,11 +3010,49 @@ dp_netdev_pmd_find_flow(const struct dp_netdev_pmd_thread *pmd, return NULL; } +static bool +dpif_netdev_get_flow_offload_status(const struct dp_netdev *dp, + const struct dp_netdev_flow *netdev_flow, + struct dpif_flow_stats *stats, + struct dpif_flow_attrs *attrs) +{ + struct nlattr *actions; + struct ofpbuf wbuffer; + struct netdev *netdev; + struct match match; + + int ret = 0; + + if (!netdev_is_flow_api_enabled()) { + return false; + } + + netdev = netdev_ports_get(netdev_flow->flow.in_port.odp_port, dp->class); + if (!netdev) { + return false; + } + /* Taking a global 'port_mutex' to fulfill thread safety + * restrictions for the netdev-offload-dpdk module. */ + ovs_mutex_lock(&dp->port_mutex); + ret = netdev_flow_get(netdev, &match, &actions, &netdev_flow->mega_ufid, + stats, attrs, &wbuffer); + ovs_mutex_unlock(&dp->port_mutex); + netdev_close(netdev); + if (ret) { + return false; + } + + return true; +} + static void -get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, - struct dpif_flow_stats *stats, bool hw) +get_dpif_flow_status(const struct dp_netdev *dp, + const struct dp_netdev_flow *netdev_flow_, + struct dpif_flow_stats *stats, + struct dpif_flow_attrs *attrs) { - struct dp_netdev_flow_stats *flow_stats; + struct dpif_flow_stats offload_stats; + struct dpif_flow_attrs offload_attrs; struct dp_netdev_flow *netdev_flow; unsigned long long n; long long used; @@ -3023,16 +3060,29 @@ get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, netdev_flow = CONST_CAST(struct dp_netdev_flow *, netdev_flow_); - flow_stats = (hw) ? &netdev_flow->hw_stats : &netdev_flow->stats; - - atomic_read_relaxed(&flow_stats->packet_count, &n); + atomic_read_relaxed(&netdev_flow->stats.packet_count, &n); stats->n_packets = n; - atomic_read_relaxed(&flow_stats->byte_count, &n); + atomic_read_relaxed(&netdev_flow->stats.byte_count, &n); stats->n_bytes = n; - atomic_read_relaxed(&flow_stats->used, &used); + atomic_read_relaxed(&netdev_flow->stats.used, &used); stats->used = used; - atomic_read_relaxed(&flow_stats->tcp_flags, &flags); + atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); stats->tcp_flags = flags; + + if (dpif_netdev_get_flow_offload_status(dp, netdev_flow, + &offload_stats, &offload_attrs)) { + stats->n_packets += offload_stats.n_packets; + stats->n_bytes += offload_stats.n_bytes; + stats->used = MAX(stats->used, offload_stats.used); + stats->tcp_flags |= offload_stats.tcp_flags; + if (attrs) { + attrs->offloaded = offload_attrs.offloaded; + attrs->dp_layer = offload_attrs.dp_layer; + } + } else if (attrs) { + attrs->offloaded = false; + attrs->dp_layer = "ovs"; + } } /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for @@ -3040,12 +3090,11 @@ get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, * 'mask_buf'. Actions will be returned without copying, by relying on RCU to * protect them. */ static void -dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow *netdev_flow, +dp_netdev_flow_to_dpif_flow(const struct dp_netdev *dp, + const struct dp_netdev_flow *netdev_flow, struct ofpbuf *key_buf, struct ofpbuf *mask_buf, - struct dpif_flow *flow, bool terse, bool offloaded) + struct dpif_flow *flow, bool terse) { - struct dpif_flow_stats hw_stats; - if (terse) { memset(flow, 0, sizeof *flow); } else { @@ -3085,14 +3134,8 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow *netdev_flow, flow->ufid = netdev_flow->ufid; flow->ufid_present = true; flow->pmd_id = netdev_flow->pmd_id; - get_dpif_flow_stats(netdev_flow, &flow->stats, false); - get_dpif_flow_stats(netdev_flow, &hw_stats, true); - flow->stats.n_packets += hw_stats.n_packets; - flow->stats.n_bytes += hw_stats.n_bytes; - flow->stats.used = MAX(flow->stats.used, hw_stats.used); - flow->attrs.offloaded = offloaded; - flow->attrs.dp_layer = flow->attrs.offloaded ? "in_hw" : "ovs"; + get_dpif_flow_status(dp, netdev_flow, &flow->stats, &flow->attrs); } static int @@ -3162,44 +3205,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len, return 0; } -static bool -dpif_netdev_offload_stats(struct dp_netdev_flow *netdev_flow, - struct dp_netdev *dp) -{ - struct dpif_flow_stats stats; - struct dpif_flow_attrs attrs; - struct nlattr *actions; - struct ofpbuf wbuffer; - struct netdev *netdev; - struct match match; - - int ret = 0; - - if (!netdev_is_flow_api_enabled()) { - return false; - } - - netdev = netdev_ports_get(netdev_flow->flow.in_port.odp_port, dp->class); - if (!netdev) { - return false; - } - /* Taking a global 'port_mutex' to fulfill thread safety - * restrictions for the netdev-offload-dpdk module. */ - ovs_mutex_lock(&dp->port_mutex); - ret = netdev_flow_get(netdev, &match, &actions, &netdev_flow->mega_ufid, - &stats, &attrs, &wbuffer); - ovs_mutex_unlock(&dp->port_mutex); - netdev_close(netdev); - if (ret) { - return false; - } - atomic_store_relaxed(&netdev_flow->hw_stats.packet_count, stats.n_packets); - atomic_store_relaxed(&netdev_flow->hw_stats.byte_count, stats.n_bytes); - atomic_store_relaxed(&netdev_flow->hw_stats.used, stats.used); - - return true; -} - static int dpif_netdev_flow_get(const struct dpif *dpif, const struct dpif_flow_get *get) { @@ -3233,9 +3238,8 @@ dpif_netdev_flow_get(const struct dpif *dpif, const struct dpif_flow_get *get) netdev_flow = dp_netdev_pmd_find_flow(pmd, get->ufid, get->key, get->key_len); if (netdev_flow) { - bool offloaded = dpif_netdev_offload_stats(netdev_flow, pmd->dp); - dp_netdev_flow_to_dpif_flow(netdev_flow, get->buffer, get->buffer, - get->flow, false, offloaded); + dp_netdev_flow_to_dpif_flow(dp, netdev_flow, get->buffer, + get->buffer, get->flow, false); error = 0; break; } else { @@ -3298,7 +3302,6 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, /* Do not allocate extra space. */ flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len); memset(&flow->stats, 0, sizeof flow->stats); - memset(&flow->hw_stats, 0, sizeof flow->hw_stats); flow->dead = false; flow->batch = NULL; flow->mark = INVALID_FLOW_MARK; @@ -3411,7 +3414,7 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, put->actions, put->actions_len); if (stats) { - get_dpif_flow_stats(netdev_flow, stats, false); + get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL); } if (put->flags & DPIF_FP_ZERO_STATS) { /* XXX: The userspace datapath uses thread local statistics @@ -3530,7 +3533,7 @@ flow_del_on_pmd(struct dp_netdev_pmd_thread *pmd, del->key_len); if (netdev_flow) { if (stats) { - get_dpif_flow_stats(netdev_flow, stats, false); + get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL); } dp_netdev_pmd_remove_flow(pmd, netdev_flow); } else { @@ -3664,16 +3667,13 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_, = dpif_netdev_flow_dump_thread_cast(thread_); struct dpif_netdev_flow_dump *dump = thread->dump; struct dp_netdev_flow *netdev_flows[FLOW_DUMP_MAX_BATCH]; - bool offloaded[FLOW_DUMP_MAX_BATCH]; + struct dpif_netdev *dpif = dpif_netdev_cast(thread->up.dpif); + struct dp_netdev *dp = get_dp_netdev(&dpif->dpif); int n_flows = 0; int i; - memset(offloaded, 0, sizeof offloaded); - ovs_mutex_lock(&dump->mutex); if (!dump->status) { - struct dpif_netdev *dpif = dpif_netdev_cast(thread->up.dpif); - struct dp_netdev *dp = get_dp_netdev(&dpif->dpif); struct dp_netdev_pmd_thread *pmd = dump->cur_pmd; int flow_limit = MIN(max_flows, FLOW_DUMP_MAX_BATCH); @@ -3699,8 +3699,6 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_, netdev_flows[n_flows] = CONTAINER_OF(node, struct dp_netdev_flow, node); - offloaded[n_flows] = - dpif_netdev_offload_stats(netdev_flows[n_flows], pmd->dp); } /* When finishing dumping the current pmd thread, moves to * the next. */ @@ -3732,8 +3730,8 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_, ofpbuf_use_stack(&key, keybuf, sizeof *keybuf); ofpbuf_use_stack(&mask, maskbuf, sizeof *maskbuf); - dp_netdev_flow_to_dpif_flow(netdev_flow, &key, &mask, f, - dump->up.terse, offloaded[i]); + dp_netdev_flow_to_dpif_flow(dp, netdev_flow, &key, &mask, f, + dump->up.terse); } return n_flows; diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 0ec840ce1..d1c066992 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -57,6 +57,7 @@ struct ufid_to_rte_flow_data { ovs_u128 ufid; struct rte_flow *rte_flow; struct dpif_flow_stats stats; + bool actions_offloaded; }; /* Find rte_flow with @ufid. */ @@ -77,7 +78,7 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid) static inline void ufid_to_rte_flow_associate(const ovs_u128 *ufid, - struct rte_flow *rte_flow) + struct rte_flow *rte_flow, bool actions_offloaded) { size_t hash = hash_bytes(ufid, sizeof *ufid, 0); struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data); @@ -96,6 +97,7 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, data->ufid = *ufid; data->rte_flow = rte_flow; + data->actions_offloaded = actions_offloaded; cmap_insert(&ufid_to_rte_flow, CONST_CAST(struct cmap_node *, &data->node), hash); @@ -1130,6 +1132,7 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, struct offload_info *info) { struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; + bool actions_offloaded = true; struct rte_flow *flow; int ret = 0; @@ -1145,13 +1148,14 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, * actions. */ flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, info->flow_mark); + actions_offloaded = false; } if (!flow) { ret = -1; goto out; } - ufid_to_rte_flow_associate(ufid, flow); + ufid_to_rte_flow_associate(ufid, flow, actions_offloaded); VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT"\n", netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); @@ -1317,7 +1321,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev, struct nlattr **actions OVS_UNUSED, const ovs_u128 *ufid, struct dpif_flow_stats *stats, - struct dpif_flow_attrs *attrs OVS_UNUSED, + struct dpif_flow_attrs *attrs, struct ofpbuf *buf OVS_UNUSED) { struct rte_flow_query_count query = { .reset = 1 }; @@ -1331,18 +1335,23 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev, goto out; } - ret = netdev_dpdk_rte_flow_query_count(netdev, rte_flow_data->rte_flow, - &query, &error); - if (ret) { - goto out; - } - rte_flow_data->stats.n_packets += (query.hits_set) ? query.hits : 0; - rte_flow_data->stats.n_bytes += (query.bytes_set) ? query.bytes : 0; - if (query.hits_set && query.hits) { - rte_flow_data->stats.used = time_usec() / 1000; + attrs->offloaded = true; + if (rte_flow_data->actions_offloaded) { + attrs->dp_layer = "dpdk"; + ret = netdev_dpdk_rte_flow_query_count(netdev, rte_flow_data->rte_flow, + &query, &error); + if (ret) { + goto out; + } + rte_flow_data->stats.n_packets += (query.hits_set) ? query.hits : 0; + rte_flow_data->stats.n_bytes += (query.bytes_set) ? query.bytes : 0; + if (query.hits_set && query.hits) { + rte_flow_data->stats.used = time_msec(); + } + } else { + attrs->dp_layer = "ovs-partial"; } memcpy(stats, &rte_flow_data->stats, sizeof *stats); - out: return ret; } -- 2.17.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev