[ovs-dev] [PATCH V3 10/12] dpif-netdev: Add mega ufid in flow add log
Ilya Maximets
i.maximets at ovn.org
Mon Jun 29 09:22:44 UTC 2020
On 6/29/20 7:33 AM, Sriharsha Basavapatna wrote:
> On Mon, Jun 29, 2020 at 5:30 AM Ilya Maximets <i.maximets at 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 at mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba at 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])
>>>
>>>
>>
More information about the dev
mailing list