diff mbox series

[ovs-dev,V3,10/12] dpif-netdev: Add mega ufid in flow add log

Message ID 20200621111924.12397-11-elibr@mellanox.com
State New
Headers show
Series netdev datapath offload: Support IPv6 and VXLAN encap | expand

Commit Message

Eli Britstein June 21, 2020, 11:19 a.m. UTC
As offload is done using the mega ufid of a flow, for better
debugability, add it in the log message.

Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
---
 lib/dpif-netdev.c       | 7 +++++--
 tests/dpif-netdev.at    | 2 ++
 tests/ofproto-macros.at | 3 ++-
 3 files changed, 9 insertions(+), 3 deletions(-)

Comments

Ilya Maximets June 29, 2020, midnight UTC | #1
On 6/21/20 1:19 PM, Eli Britstein wrote:
> As offload is done using the mega ufid of a flow, for better
> debugability, add it in the log message.

Could you, please, tell me why we need this extra ufid generated
at all?  Why the usual flow ufid is not enough?  I'm a bit confused.

> 
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> ---
>  lib/dpif-netdev.c       | 7 +++++--
>  tests/dpif-netdev.at    | 2 ++
>  tests/ofproto-macros.at | 3 ++-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 57565802a..da0c48ef5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2510,8 +2510,9 @@ dp_netdev_flow_offload_main(void *data OVS_UNUSED)
>              OVS_NOT_REACHED();
>          }
>  
> -        VLOG_DBG("%s to %s netdev flow\n",
> -                 ret == 0 ? "succeed" : "failed", op);
> +        VLOG_DBG("%s to %s netdev flow "UUID_FMT"\n",
> +                 ret == 0 ? "succeed" : "failed", op,
> +                 UUID_ARGS((struct uuid *) &offload->flow->mega_ufid));
>          dp_netdev_free_flow_offload(offload);
>          ovsrcu_quiesce();
>      }
> @@ -3383,6 +3384,8 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>  
>          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,
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 21f0c8d24..ec5ffc290 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -13,6 +13,7 @@ strip_timers () {
>  
>  strip_xout () {
>      sed '
> +    s/mega_ufid:[-0-9a-f]* //
>      s/ufid:[-0-9a-f]* //
>      s/used:[0-9]*\.[0-9]*/used:0.0/
>      s/actions:.*/actions: <del>/
> @@ -23,6 +24,7 @@ strip_xout () {
>  
>  strip_xout_keep_actions () {
>      sed '
> +    s/mega_ufid:[-0-9a-f]* //
>      s/ufid:[-0-9a-f]* //
>      s/used:[0-9]*\.[0-9]*/used:0.0/
>      s/packets:[0-9]*/packets:0/
> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> index b2b17eed3..87f9ae280 100644
> --- a/tests/ofproto-macros.at
> +++ b/tests/ofproto-macros.at
> @@ -131,7 +131,8 @@ strip_duration () {
>  # Strips 'ufid:...' from output, to make it easier to compare.
>  # (ufids are random.)
>  strip_ufid () {
> -    sed 's/ufid:[[-0-9a-f]]* //'
> +    sed 's/mega_ufid:[[-0-9a-f]]* //
> +    s/ufid:[[-0-9a-f]]* //'
>  }
>  m4_divert_pop([PREPARE_TESTS])
>  
>
Sriharsha Basavapatna June 29, 2020, 5:33 a.m. UTC | #2
On Mon, Jun 29, 2020 at 5:30 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 6/21/20 1:19 PM, Eli Britstein wrote:
> > As offload is done using the mega ufid of a flow, for better
> > debugability, add it in the log message.
>
> Could you, please, tell me why we need this extra ufid generated
> at all?  Why the usual flow ufid is not enough?  I'm a bit confused.

I believe this comes from the mark/rss (partial offload)
implementation. The rationale behind this design is explained in this
commit:

241bad15d99a422dce71a6ec6116a2d7b9c31796
(dpif-netdev: associate flow with a mark id)

I'm pasting the relevant section here for quick reference:

"   One tricky thing in OVS-DPDK is, the flow tables is per-PMD. For the
    case there is only one phys port but with 2 queues, there could be 2
    PMDs. In other words, even for a single mega flow (i.e. udp,tp_src=1000),
    there could be 2 different dp_netdev flows, one for each PMD. That could
    results to the same mega flow being offloaded twice in the hardware,
    worse, we may get 2 different marks and only the last one will work.

    To avoid that, a megaflow_to_mark CMAP is created. An entry will be
    added for the first PMD that wants to offload a flow. For later PMDs,
    it will see such megaflow is already offloaded, then the flow will not
    be offloaded to HW twice."

Thanks,
-Harsha

>
> >
> > Signed-off-by: Eli Britstein <elibr@mellanox.com>
> > Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> > ---
> >  lib/dpif-netdev.c       | 7 +++++--
> >  tests/dpif-netdev.at    | 2 ++
> >  tests/ofproto-macros.at | 3 ++-
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 57565802a..da0c48ef5 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2510,8 +2510,9 @@ dp_netdev_flow_offload_main(void *data OVS_UNUSED)
> >              OVS_NOT_REACHED();
> >          }
> >
> > -        VLOG_DBG("%s to %s netdev flow\n",
> > -                 ret == 0 ? "succeed" : "failed", op);
> > +        VLOG_DBG("%s to %s netdev flow "UUID_FMT"\n",
> > +                 ret == 0 ? "succeed" : "failed", op,
> > +                 UUID_ARGS((struct uuid *) &offload->flow->mega_ufid));
> >          dp_netdev_free_flow_offload(offload);
> >          ovsrcu_quiesce();
> >      }
> > @@ -3383,6 +3384,8 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> >
> >          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,
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index 21f0c8d24..ec5ffc290 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -13,6 +13,7 @@ strip_timers () {
> >
> >  strip_xout () {
> >      sed '
> > +    s/mega_ufid:[-0-9a-f]* //
> >      s/ufid:[-0-9a-f]* //
> >      s/used:[0-9]*\.[0-9]*/used:0.0/
> >      s/actions:.*/actions: <del>/
> > @@ -23,6 +24,7 @@ strip_xout () {
> >
> >  strip_xout_keep_actions () {
> >      sed '
> > +    s/mega_ufid:[-0-9a-f]* //
> >      s/ufid:[-0-9a-f]* //
> >      s/used:[0-9]*\.[0-9]*/used:0.0/
> >      s/packets:[0-9]*/packets:0/
> > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> > index b2b17eed3..87f9ae280 100644
> > --- a/tests/ofproto-macros.at
> > +++ b/tests/ofproto-macros.at
> > @@ -131,7 +131,8 @@ strip_duration () {
> >  # Strips 'ufid:...' from output, to make it easier to compare.
> >  # (ufids are random.)
> >  strip_ufid () {
> > -    sed 's/ufid:[[-0-9a-f]]* //'
> > +    sed 's/mega_ufid:[[-0-9a-f]]* //
> > +    s/ufid:[[-0-9a-f]]* //'
> >  }
> >  m4_divert_pop([PREPARE_TESTS])
> >
> >
>
Ilya Maximets June 29, 2020, 9:22 a.m. UTC | #3
On 6/29/20 7:33 AM, Sriharsha Basavapatna wrote:
> On Mon, Jun 29, 2020 at 5:30 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> On 6/21/20 1:19 PM, Eli Britstein wrote:
>>> As offload is done using the mega ufid of a flow, for better
>>> debugability, add it in the log message.
>>
>> Could you, please, tell me why we need this extra ufid generated
>> at all?  Why the usual flow ufid is not enough?  I'm a bit confused.
> 
> I believe this comes from the mark/rss (partial offload)
> implementation. The rationale behind this design is explained in this
> commit:
> 
> 241bad15d99a422dce71a6ec6116a2d7b9c31796
> (dpif-netdev: associate flow with a mark id)
> 
> I'm pasting the relevant section here for quick reference:
> 
> "   One tricky thing in OVS-DPDK is, the flow tables is per-PMD. For the
>     case there is only one phys port but with 2 queues, there could be 2
>     PMDs. In other words, even for a single mega flow (i.e. udp,tp_src=1000),
>     there could be 2 different dp_netdev flows, one for each PMD. That could
>     results to the same mega flow being offloaded twice in the hardware,
>     worse, we may get 2 different marks and only the last one will work.
> 
>     To avoid that, a megaflow_to_mark CMAP is created. An entry will be
>     added for the first PMD that wants to offload a flow. For later PMDs,
>     it will see such megaflow is already offloaded, then the flow will not
>     be offloaded to HW twice."

OK.  Thanks for the pointer!

That is because we're mixing pmd_id into flow ufid.  I see.
There should be a more elegant solution, but let it be this way for now.

> 
> Thanks,
> -Harsha
> 
>>
>>>
>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>>> ---
>>>  lib/dpif-netdev.c       | 7 +++++--
>>>  tests/dpif-netdev.at    | 2 ++
>>>  tests/ofproto-macros.at | 3 ++-
>>>  3 files changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 57565802a..da0c48ef5 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -2510,8 +2510,9 @@ dp_netdev_flow_offload_main(void *data OVS_UNUSED)
>>>              OVS_NOT_REACHED();
>>>          }
>>>
>>> -        VLOG_DBG("%s to %s netdev flow\n",
>>> -                 ret == 0 ? "succeed" : "failed", op);
>>> +        VLOG_DBG("%s to %s netdev flow "UUID_FMT"\n",
>>> +                 ret == 0 ? "succeed" : "failed", op,
>>> +                 UUID_ARGS((struct uuid *) &offload->flow->mega_ufid));
>>>          dp_netdev_free_flow_offload(offload);
>>>          ovsrcu_quiesce();
>>>      }
>>> @@ -3383,6 +3384,8 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>>>
>>>          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,
>>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
>>> index 21f0c8d24..ec5ffc290 100644
>>> --- a/tests/dpif-netdev.at
>>> +++ b/tests/dpif-netdev.at
>>> @@ -13,6 +13,7 @@ strip_timers () {
>>>
>>>  strip_xout () {
>>>      sed '
>>> +    s/mega_ufid:[-0-9a-f]* //
>>>      s/ufid:[-0-9a-f]* //
>>>      s/used:[0-9]*\.[0-9]*/used:0.0/
>>>      s/actions:.*/actions: <del>/
>>> @@ -23,6 +24,7 @@ strip_xout () {
>>>
>>>  strip_xout_keep_actions () {
>>>      sed '
>>> +    s/mega_ufid:[-0-9a-f]* //
>>>      s/ufid:[-0-9a-f]* //
>>>      s/used:[0-9]*\.[0-9]*/used:0.0/
>>>      s/packets:[0-9]*/packets:0/
>>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
>>> index b2b17eed3..87f9ae280 100644
>>> --- a/tests/ofproto-macros.at
>>> +++ b/tests/ofproto-macros.at
>>> @@ -131,7 +131,8 @@ strip_duration () {
>>>  # Strips 'ufid:...' from output, to make it easier to compare.
>>>  # (ufids are random.)
>>>  strip_ufid () {
>>> -    sed 's/ufid:[[-0-9a-f]]* //'
>>> +    sed 's/mega_ufid:[[-0-9a-f]]* //
>>> +    s/ufid:[[-0-9a-f]]* //'
>>>  }
>>>  m4_divert_pop([PREPARE_TESTS])
>>>
>>>
>>
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 57565802a..da0c48ef5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2510,8 +2510,9 @@  dp_netdev_flow_offload_main(void *data OVS_UNUSED)
             OVS_NOT_REACHED();
         }
 
-        VLOG_DBG("%s to %s netdev flow\n",
-                 ret == 0 ? "succeed" : "failed", op);
+        VLOG_DBG("%s to %s netdev flow "UUID_FMT"\n",
+                 ret == 0 ? "succeed" : "failed", op,
+                 UUID_ARGS((struct uuid *) &offload->flow->mega_ufid));
         dp_netdev_free_flow_offload(offload);
         ovsrcu_quiesce();
     }
@@ -3383,6 +3384,8 @@  dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
 
         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,
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 21f0c8d24..ec5ffc290 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -13,6 +13,7 @@  strip_timers () {
 
 strip_xout () {
     sed '
+    s/mega_ufid:[-0-9a-f]* //
     s/ufid:[-0-9a-f]* //
     s/used:[0-9]*\.[0-9]*/used:0.0/
     s/actions:.*/actions: <del>/
@@ -23,6 +24,7 @@  strip_xout () {
 
 strip_xout_keep_actions () {
     sed '
+    s/mega_ufid:[-0-9a-f]* //
     s/ufid:[-0-9a-f]* //
     s/used:[0-9]*\.[0-9]*/used:0.0/
     s/packets:[0-9]*/packets:0/
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index b2b17eed3..87f9ae280 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -131,7 +131,8 @@  strip_duration () {
 # Strips 'ufid:...' from output, to make it easier to compare.
 # (ufids are random.)
 strip_ufid () {
-    sed 's/ufid:[[-0-9a-f]]* //'
+    sed 's/mega_ufid:[[-0-9a-f]]* //
+    s/ufid:[[-0-9a-f]]* //'
 }
 m4_divert_pop([PREPARE_TESTS])