diff mbox series

[ovs-dev,V2,3/3] dpif-netdev: Log flow modification in debug level

Message ID 20210712150710.19197-4-elibr@nvidia.com
State Changes Requested
Headers show
Series dpif-netdev offload transitions | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot success github build: passed

Commit Message

Eli Britstein July 12, 2021, 3:07 p.m. UTC 
Log flow modifications to help debugging.

Signed-off-by: Eli Britstein <elibr@nvidia.com>
Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
---
 lib/dpif-netdev.c | 101 +++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 46 deletions(-)

Comments

Ilya Maximets July 25, 2021, 11:42 p.m. UTC | #1 
On 7/12/21 5:07 PM, Eli Britstein wrote:
> Log flow modifications to help debugging.
> 
> Signed-off-by: Eli Britstein <elibr@nvidia.com>
> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
> ---
>  lib/dpif-netdev.c | 101 +++++++++++++++++++++++++---------------------
>  1 file changed, 55 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 9b2b8d6d9..caed3e7f2 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2457,6 +2457,61 @@ queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
>      struct dp_flow_offload_item *offload;
>      int op;
>  
> +    if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) {
> +        struct ds ds = DS_EMPTY_INITIALIZER;
> +        struct ofpbuf key_buf, mask_buf;
> +        struct odp_flow_key_parms odp_parms = {
> +            .flow = &match->flow,
> +            .mask = &match->wc.masks,
> +            .support = dp_netdev_support,
> +        };
> +
> +        ofpbuf_init(&key_buf, 0);
> +        ofpbuf_init(&mask_buf, 0);
> +
> +        odp_flow_key_from_flow(&odp_parms, &key_buf);
> +        odp_parms.key_buf = &key_buf;
> +        odp_flow_key_from_mask(&odp_parms, &mask_buf);
> +
> +        if (old_actions) {
> +            ds_put_cstr(&ds, "flow_mod: ");
> +        } else {
> +            ds_put_cstr(&ds, "flow_add: ");
> +        }
> +        odp_format_ufid(&flow->ufid, &ds);
> +        ds_put_cstr(&ds, " mega_");
> +        odp_format_ufid(&flow->mega_ufid, &ds);
> +        ds_put_cstr(&ds, " ");
> +        odp_flow_format(key_buf.data, key_buf.size,
> +                        mask_buf.data, mask_buf.size,
> +                        NULL, &ds, false);
> +        if (old_actions) {
> +            ds_put_cstr(&ds, ", old_actions:");
> +            format_odp_actions(&ds, old_actions->actions, old_actions->size,
> +                               NULL);
> +        }
> +        ds_put_cstr(&ds, ", actions:");
> +        format_odp_actions(&ds, actions, actions_len, NULL);
> +
> +        VLOG_DBG("%s", ds_cstr(&ds));
> +
> +        ofpbuf_uninit(&key_buf);
> +        ofpbuf_uninit(&mask_buf);
> +
> +        /* Add a printout of the actual match installed. */
> +        struct match m;
> +        ds_clear(&ds);
> +        ds_put_cstr(&ds, "flow match: ");
> +        miniflow_expand(&flow->cr.flow.mf, &m.flow);
> +        miniflow_expand(&flow->cr.mask->mf, &m.wc.masks);
> +        memset(&m.tun_md, 0, sizeof m.tun_md);
> +        match_format(&m, NULL, &ds, OFP_DEFAULT_PRIORITY);
> +
> +        VLOG_DBG("%s", ds_cstr(&ds));
> +
> +        ds_destroy(&ds);
> +    }
> +

It make sense to print.  However, I don't think that this generic
log belongs to queue_netdev_flow_put() function.  It shouldn't be
there.  Suggesting to create a separate function for logging and
call it in two places after queue_netdev_flow_put().

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 9b2b8d6d9..caed3e7f2 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2457,6 +2457,61 @@  queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
     struct dp_flow_offload_item *offload;
     int op;
 
+    if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) {
+        struct ds ds = DS_EMPTY_INITIALIZER;
+        struct ofpbuf key_buf, mask_buf;
+        struct odp_flow_key_parms odp_parms = {
+            .flow = &match->flow,
+            .mask = &match->wc.masks,
+            .support = dp_netdev_support,
+        };
+
+        ofpbuf_init(&key_buf, 0);
+        ofpbuf_init(&mask_buf, 0);
+
+        odp_flow_key_from_flow(&odp_parms, &key_buf);
+        odp_parms.key_buf = &key_buf;
+        odp_flow_key_from_mask(&odp_parms, &mask_buf);
+
+        if (old_actions) {
+            ds_put_cstr(&ds, "flow_mod: ");
+        } else {
+            ds_put_cstr(&ds, "flow_add: ");
+        }
+        odp_format_ufid(&flow->ufid, &ds);
+        ds_put_cstr(&ds, " mega_");
+        odp_format_ufid(&flow->mega_ufid, &ds);
+        ds_put_cstr(&ds, " ");
+        odp_flow_format(key_buf.data, key_buf.size,
+                        mask_buf.data, mask_buf.size,
+                        NULL, &ds, false);
+        if (old_actions) {
+            ds_put_cstr(&ds, ", old_actions:");
+            format_odp_actions(&ds, old_actions->actions, old_actions->size,
+                               NULL);
+        }
+        ds_put_cstr(&ds, ", actions:");
+        format_odp_actions(&ds, actions, actions_len, NULL);
+
+        VLOG_DBG("%s", ds_cstr(&ds));
+
+        ofpbuf_uninit(&key_buf);
+        ofpbuf_uninit(&mask_buf);
+
+        /* Add a printout of the actual match installed. */
+        struct match m;
+        ds_clear(&ds);
+        ds_put_cstr(&ds, "flow match: ");
+        miniflow_expand(&flow->cr.flow.mf, &m.flow);
+        miniflow_expand(&flow->cr.mask->mf, &m.wc.masks);
+        memset(&m.tun_md, 0, sizeof m.tun_md);
+        match_format(&m, NULL, &ds, OFP_DEFAULT_PRIORITY);
+
+        VLOG_DBG("%s", ds_cstr(&ds));
+
+        ds_destroy(&ds);
+    }
+
     if (!netdev_is_flow_api_enabled()) {
         return;
     }
@@ -3324,52 +3379,6 @@  dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
     queue_netdev_flow_put(pmd, flow, match, actions, actions_len,
                           orig_in_port, NULL);
 
-    if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) {
-        struct ds ds = DS_EMPTY_INITIALIZER;
-        struct ofpbuf key_buf, mask_buf;
-        struct odp_flow_key_parms odp_parms = {
-            .flow = &match->flow,
-            .mask = &match->wc.masks,
-            .support = dp_netdev_support,
-        };
-
-        ofpbuf_init(&key_buf, 0);
-        ofpbuf_init(&mask_buf, 0);
-
-        odp_flow_key_from_flow(&odp_parms, &key_buf);
-        odp_parms.key_buf = &key_buf;
-        odp_flow_key_from_mask(&odp_parms, &mask_buf);
-
-        ds_put_cstr(&ds, "flow_add: ");
-        odp_format_ufid(ufid, &ds);
-        ds_put_cstr(&ds, " mega_");
-        odp_format_ufid(&flow->mega_ufid, &ds);
-        ds_put_cstr(&ds, " ");
-        odp_flow_format(key_buf.data, key_buf.size,
-                        mask_buf.data, mask_buf.size,
-                        NULL, &ds, false);
-        ds_put_cstr(&ds, ", actions:");
-        format_odp_actions(&ds, actions, actions_len, NULL);
-
-        VLOG_DBG("%s", ds_cstr(&ds));
-
-        ofpbuf_uninit(&key_buf);
-        ofpbuf_uninit(&mask_buf);
-
-        /* Add a printout of the actual match installed. */
-        struct match m;
-        ds_clear(&ds);
-        ds_put_cstr(&ds, "flow match: ");
-        miniflow_expand(&flow->cr.flow.mf, &m.flow);
-        miniflow_expand(&flow->cr.mask->mf, &m.wc.masks);
-        memset(&m.tun_md, 0, sizeof m.tun_md);
-        match_format(&m, NULL, &ds, OFP_DEFAULT_PRIORITY);
-
-        VLOG_DBG("%s", ds_cstr(&ds));
-
-        ds_destroy(&ds);
-    }
-
     return flow;
 }