| Message ID | 20200621111924.12397-11-elibr@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev datapath offload: Support IPv6 and VXLAN encap | expand |
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]) > >
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]) > > > > >
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 --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])