diff mbox series

[ovs-dev] Avoid indeterminate statistics in offload implementations.

Message ID 20191025184624.26278-1-blp@ovn.org
State Accepted
Commit 75ad1cd6e94a06d09646ebd03097eedf39b0c6db
Headers show
Series [ovs-dev] Avoid indeterminate statistics in offload implementations. | expand

Commit Message

Ben Pfaff Oct. 25, 2019, 6:46 p.m. UTC 
A lot of the offload implementations didn't bother to initialize the
statistics they were supposed to return.  I don't know whether any of
the callers actually use them, but it looked wrong.

Found by inspection.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/netdev-dummy.c        | 10 ++++++++--
 lib/netdev-offload-dpdk.c | 10 ++++++++--
 lib/netdev-offload-tc.c   |  5 ++++-
 3 files changed, 20 insertions(+), 5 deletions(-)

Comments

Ilya Maximets Oct. 30, 2019, 7:40 p.m. UTC | #1 
On 25.10.2019 20:46, Ben Pfaff wrote:
> A lot of the offload implementations didn't bother to initialize the
> statistics they were supposed to return.  I don't know whether any of
> the callers actually use them, but it looked wrong.
> 
> Found by inspection.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>   lib/netdev-dummy.c        | 10 ++++++++--
>   lib/netdev-offload-dpdk.c | 10 ++++++++--
>   lib/netdev-offload-tc.c   |  5 ++++-
>   3 files changed, 20 insertions(+), 5 deletions(-)

This looks correct.
'dummy' and 'dpdk' implementations doesn't support stats at all, so
these should be cleared. 'tc' needs some more work to return stats
on flow replacing, but this should be a separate change.

Acked-by: Ilya Maximets <i.maximets@ovn.org>
Ben Pfaff Oct. 31, 2019, 5:07 p.m. UTC | #2 
On Wed, Oct 30, 2019 at 08:40:22PM +0100, Ilya Maximets wrote:
> On 25.10.2019 20:46, Ben Pfaff wrote:
> > A lot of the offload implementations didn't bother to initialize the
> > statistics they were supposed to return.  I don't know whether any of
> > the callers actually use them, but it looked wrong.
> > 
> > Found by inspection.
> > 
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >   lib/netdev-dummy.c        | 10 ++++++++--
> >   lib/netdev-offload-dpdk.c | 10 ++++++++--
> >   lib/netdev-offload-tc.c   |  5 ++++-
> >   3 files changed, 20 insertions(+), 5 deletions(-)
> 
> This looks correct.
> 'dummy' and 'dpdk' implementations doesn't support stats at all, so
> these should be cleared. 'tc' needs some more work to return stats
> on flow replacing, but this should be a separate change.
> 
> Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thanks, applied to master and branch-2.12.
diff mbox series

Patch

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 95e1a329a908..71df29184d9b 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1434,7 +1434,7 @@  netdev_dummy_flow_put(struct netdev *netdev, struct match *match,
                       struct nlattr *actions OVS_UNUSED,
                       size_t actions_len OVS_UNUSED,
                       const ovs_u128 *ufid, struct offload_info *info,
-                      struct dpif_flow_stats *stats OVS_UNUSED)
+                      struct dpif_flow_stats *stats)
 {
     struct netdev_dummy *dev = netdev_dummy_cast(netdev);
     struct offloaded_flow *off_flow;
@@ -1476,12 +1476,15 @@  netdev_dummy_flow_put(struct netdev *netdev, struct match *match,
         ds_destroy(&ds);
     }
 
+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+    }
     return 0;
 }
 
 static int
 netdev_dummy_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
-                      struct dpif_flow_stats *stats OVS_UNUSED)
+                      struct dpif_flow_stats *stats)
 {
     struct netdev_dummy *dev = netdev_dummy_cast(netdev);
     struct offloaded_flow *off_flow;
@@ -1521,6 +1524,9 @@  exit:
         ds_destroy(&ds);
     }
 
+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+    }
     return error ? -1 : 0;
 }
 
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 01e900461adf..96794dc4dc8b 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -710,7 +710,7 @@  static int
 netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
                              struct nlattr *actions, size_t actions_len,
                              const ovs_u128 *ufid, struct offload_info *info,
-                             struct dpif_flow_stats *stats OVS_UNUSED)
+                             struct dpif_flow_stats *stats)
 {
     struct rte_flow *rte_flow;
     int ret;
@@ -732,13 +732,16 @@  netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
         return ret;
     }
 
+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+    }
     return netdev_offload_dpdk_add_flow(netdev, match, actions,
                                         actions_len, ufid, info);
 }
 
 static int
 netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
-                             struct dpif_flow_stats *stats OVS_UNUSED)
+                             struct dpif_flow_stats *stats)
 {
     struct rte_flow *rte_flow = ufid_to_rte_flow_find(ufid);
 
@@ -746,6 +749,9 @@  netdev_offload_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
         return -1;
     }
 
+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+    }
     return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
 }
 
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index f6d1abb2e695..c5b9cbdb2bfe 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1143,7 +1143,7 @@  static int
 netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                    struct nlattr *actions, size_t actions_len,
                    const ovs_u128 *ufid, struct offload_info *info,
-                   struct dpif_flow_stats *stats OVS_UNUSED)
+                   struct dpif_flow_stats *stats)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
     enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
@@ -1448,6 +1448,9 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
 
     err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
     if (!err) {
+        if (stats) {
+            memset(stats, 0, sizeof *stats);
+        }
         add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
     }