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

2021-07-25 Thread Ilya Maximets 
On 7/12/21 5:07 PM, Eli Britstein wrote:
> Log flow modifications to help debugging.
> 
> Signed-off-by: Eli Britstein 
> Reviewed-by: Gaetan Rivet 
> ---
>  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((_rl {
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +struct ofpbuf key_buf, mask_buf;
> +struct odp_flow_key_parms odp_parms = {
> +.flow = >flow,
> +.mask = >wc.masks,
> +.support = dp_netdev_support,
> +};
> +
> +ofpbuf_init(_buf, 0);
> +ofpbuf_init(_buf, 0);
> +
> +odp_flow_key_from_flow(_parms, _buf);
> +odp_parms.key_buf = _buf;
> +odp_flow_key_from_mask(_parms, _buf);
> +
> +if (old_actions) {
> +ds_put_cstr(, "flow_mod: ");
> +} else {
> +ds_put_cstr(, "flow_add: ");
> +}
> +odp_format_ufid(>ufid, );
> +ds_put_cstr(, " mega_");
> +odp_format_ufid(>mega_ufid, );
> +ds_put_cstr(, " ");
> +odp_flow_format(key_buf.data, key_buf.size,
> +mask_buf.data, mask_buf.size,
> +NULL, , false);
> +if (old_actions) {
> +ds_put_cstr(, ", old_actions:");
> +format_odp_actions(, old_actions->actions, old_actions->size,
> +   NULL);
> +}
> +ds_put_cstr(, ", actions:");
> +format_odp_actions(, actions, actions_len, NULL);
> +
> +VLOG_DBG("%s", ds_cstr());
> +
> +ofpbuf_uninit(_buf);
> +ofpbuf_uninit(_buf);
> +
> +/* Add a printout of the actual match installed. */
> +struct match m;
> +ds_clear();
> +ds_put_cstr(, "flow match: ");
> +miniflow_expand(>cr.flow.mf, );
> +miniflow_expand(>cr.mask->mf, );
> +memset(_md, 0, sizeof m.tun_md);
> +match_format(, NULL, , OFP_DEFAULT_PRIORITY);
> +
> +VLOG_DBG("%s", ds_cstr());
> +
> +ds_destroy();
> +}
> +

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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2021-07-12 Thread Eli Britstein 
Log flow modifications to help debugging.

Signed-off-by: Eli Britstein 
Reviewed-by: Gaetan Rivet 
---
 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((_rl {
+struct ds ds = DS_EMPTY_INITIALIZER;
+struct ofpbuf key_buf, mask_buf;
+struct odp_flow_key_parms odp_parms = {
+.flow = >flow,
+.mask = >wc.masks,
+.support = dp_netdev_support,
+};
+
+ofpbuf_init(_buf, 0);
+ofpbuf_init(_buf, 0);
+
+odp_flow_key_from_flow(_parms, _buf);
+odp_parms.key_buf = _buf;
+odp_flow_key_from_mask(_parms, _buf);
+
+if (old_actions) {
+ds_put_cstr(, "flow_mod: ");
+} else {
+ds_put_cstr(, "flow_add: ");
+}
+odp_format_ufid(>ufid, );
+ds_put_cstr(, " mega_");
+odp_format_ufid(>mega_ufid, );
+ds_put_cstr(, " ");
+odp_flow_format(key_buf.data, key_buf.size,
+mask_buf.data, mask_buf.size,
+NULL, , false);
+if (old_actions) {
+ds_put_cstr(, ", old_actions:");
+format_odp_actions(, old_actions->actions, old_actions->size,
+   NULL);
+}
+ds_put_cstr(, ", actions:");
+format_odp_actions(, actions, actions_len, NULL);
+
+VLOG_DBG("%s", ds_cstr());
+
+ofpbuf_uninit(_buf);
+ofpbuf_uninit(_buf);
+
+/* Add a printout of the actual match installed. */
+struct match m;
+ds_clear();
+ds_put_cstr(, "flow match: ");
+miniflow_expand(>cr.flow.mf, );
+miniflow_expand(>cr.mask->mf, );
+memset(_md, 0, sizeof m.tun_md);
+match_format(, NULL, , OFP_DEFAULT_PRIORITY);
+
+VLOG_DBG("%s", ds_cstr());
+
+ds_destroy();
+}
+
 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((_rl {
-struct ds ds = DS_EMPTY_INITIALIZER;
-struct ofpbuf key_buf, mask_buf;
-struct odp_flow_key_parms odp_parms = {
-.flow = >flow,
-.mask = >wc.masks,
-.support = dp_netdev_support,
-};
-
-ofpbuf_init(_buf, 0);
-ofpbuf_init(_buf, 0);
-
-odp_flow_key_from_flow(_parms, _buf);
-odp_parms.key_buf = _buf;
-odp_flow_key_from_mask(_parms, _buf);
-
-ds_put_cstr(, "flow_add: ");
-odp_format_ufid(ufid, );
-ds_put_cstr(, " mega_");
-odp_format_ufid(>mega_ufid, );
-ds_put_cstr(, " ");
-odp_flow_format(key_buf.data, key_buf.size,
-mask_buf.data, mask_buf.size,
-NULL, , false);
-ds_put_cstr(, ", actions:");
-format_odp_actions(, actions, actions_len, NULL);
-
-VLOG_DBG("%s", ds_cstr());
-
-ofpbuf_uninit(_buf);
-ofpbuf_uninit(_buf);
-
-/* Add a printout of the actual match installed. */
-struct match m;
-ds_clear();
-ds_put_cstr(, "flow match: ");
-miniflow_expand(>cr.flow.mf, );
-miniflow_expand(>cr.mask->mf, );
-memset(_md, 0, sizeof m.tun_md);
-match_format(, NULL, , OFP_DEFAULT_PRIORITY);
-
-VLOG_DBG("%s", ds_cstr());
-
-ds_destroy();
-}
-
 return flow;
 }
 
-- 
2.28.0.2311.g225365fb51

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev