[ovs-dev] [PATCH v2] dpif-netlink: Fix issues of the offloaded flows counter
Jianbo Liu
jianbol at mellanox.com
Thu Dec 17 01:26:51 UTC 2020
The 12/16/2020 13:17, Ilya Maximets wrote:
> On 12/15/20 7:30 AM, Jianbo Liu wrote:
> > 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.
>
> Thanks, this looks like a cleaner solution.
> I didn't read the patch very carefully, but it looks OK at a quick glance.
> Some comments inline.
Thanks. I will address them all in v3, please continue review it.
>
> Bets regards, Ilya Maximets.
>
> >>
> >> 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)
>
> Shouldn't this be dpif_get_n_offloaded_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) {
>
> We're not modifying the hash map here, so it should be simple HMAP_FOR_EACH
> iterator.
>
> >> + if (data->netdev != netdev) {
> >> + continue;
> >> + }
> >> + total++;
>
> Maybe, it's better to turn 'if' condition to positive one and save some lines?
> e.g.,
>
> if (data->netdev == netdev) {
> 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,
>
> 'dpif_port' is here only for the 'port_no'. It's , probably, better, to just pass
> 'port_no' to this function directly.
>
> >> + 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);
>
> This causes build failure on 32bit:
> ofproto/ofproto-dpif-upcall.c:2920:55: error: format ‘%ld’ expects argument of type
> ‘long int’, but argument 3 has type ‘uint64_t {aka long long unsigned int}’ [-Werror=format=]
> ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows);
> ~~^
> %lld
>
> You should use PRIu64 instead.
>
> >> + }
> >> 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.
>
> Maybe this could be:
>
> AT_CHECK([test $(ovs-appctl upcall/show | grep -c "offloaded flows") -eq 0 ])
>
> ?
>
> >
> >> +
> >> 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.
>
> Yeah, there could be some random traffic, since it's a system test, e.g. some ARP
> and ipv6 packets. If you want a precise check, you could still add an implementation
> for netdev_offload_dummy and add some checks to 'dpif-netdev - partial hw offload' tests
> in tests/dpif-netdev.at. It supports only partial offloading, but I don't think that
> really matters here.
--
More information about the dev
mailing list