On 12/10/20 9:43 AM, Roi Dayan wrote:
> From: Jianbo Liu <jian...@nvidia.com>
>
Hi.
This is not a full review. Some comments inline.
Best regards, Ilya Maximets.
> 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-flows, dpctl/del-flow and
> dpctl/del-flows, a new dpif is opened, so n_offloaded_flows in it
> can't be used. To fix this, move n_offloaded_flows to dp_netlink.
>
> Besides, this stats is set to zero for userspace datapath as it is
> intented for counting the rules offloaded by tc flower
Why it is limited to TC flower? It has very generic name that could
mean flows offloaded with rte_flow or dummy offload provider.
>
> Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules")
> Signed-off-by: Jianbo Liu <jian...@nvidia.com>
> Reviewed-by: Roi Dayan <r...@nvidia.com>
> ---
> lib/dpif-netdev.c | 1 +
> lib/dpif-netlink.c | 11 +++++++----
> tests/system-offloads-traffic.at | 4 ++++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 300861ca5972..3b1c06b7d6f5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2023,6 +2023,7 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct
> dpif_dp_stats *stats)
> }
> stats->n_masks = UINT32_MAX;
> stats->n_mask_hit = UINT64_MAX;
> + stats->n_offloaded_flows = 0;
This should not be zero, because this will give misleading information
that no flows offloaded while netdev_offload_dpdk or netdev_offload_dummy
is in use.
At the same time TC ofloading could be used by userspace datapath and work
fine in skip_sw mode. And this counter should not report 0 in this case
too.
At least, you could set it to UINT64_MAX and use this value as indicator
that this counter is not supported. And not print it to the user.
>
> return 0;
> }
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 8a7b5a91fe4f..25db4a3ddef2 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -202,6 +202,8 @@ struct dp_netlink {
> const struct dpif_class *const class;
> const char *const name;
> struct ovs_refcount ref_cnt;
> +
> + struct atomic_count n_offloaded_flows;
> };
>
> /* Datapath interface for the openvswitch Linux kernel module. */
> @@ -221,7 +223,6 @@ struct dpif_netlink {
> /* Change notification. */
> struct nl_sock *port_notifier; /* vport multicast group subscriber. */
> bool refresh_channels;
> - struct atomic_count n_offloaded_flows;
> struct dp_netlink *dp;
> };
>
> @@ -752,7 +753,7 @@ 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);
> + stats->n_offloaded_flows =
> atomic_count_get(&dpif->dp->n_offloaded_flows);
> return error;
> }
>
> @@ -1205,6 +1206,7 @@ dpif_netlink_flow_flush(struct dpif *dpif_)
>
> if (netdev_is_flow_api_enabled()) {
> netdev_ports_flow_flush(dpif_type_str);
> + atomic_count_set(&dpif->dp->n_offloaded_flows, 0);
> }
>
> return dpif_netlink_flow_transact(&flow, NULL, NULL);
> @@ -2253,6 +2255,7 @@ out:
> static int
> try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
> {
> + struct dp_netlink *dp = dpif->dp;
> int err = EOPNOTSUPP;
>
> switch (op->type) {
> @@ -2265,7 +2268,7 @@ 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);
> + atomic_count_inc(&dp->n_offloaded_flows);
> }
> log_flow_put_message(&dpif->dpif, &this_module, put, 0);
> break;
> @@ -2282,7 +2285,7 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct
> dpif_op *op)
> del->ufid,
> del->stats);
> if (!err) {
> - atomic_count_dec(&dpif->n_offloaded_flows);
> + atomic_count_dec(&dp->n_offloaded_flows);
> }
> log_flow_del_message(&dpif->dpif, &this_module, del, 0);
> break;
> diff --git a/tests/system-offloads-traffic.at
> b/tests/system-offloads-traffic.at
> index 379a8a5e9280..a466755f931a 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"], [0],
> [ignore])
> +
> 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])
Why not the exact value here?
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
