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.
Bets regards, Ilya Maximets.
>>
>> Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules")
>> Signed-off-by: Jianbo Liu <[email protected]>
>> ---
>> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
