[ovs-dev] [PATCH ovn v3 1/2] lflow.c: Avoid adding redundant resource refs for port-bindings.

Han Zhou hzhou at ovn.org
Thu Sep 17 17:56:13 UTC 2020


On Thu, Sep 17, 2020 at 5:44 AM Numan Siddique <numans at ovn.org> wrote:
>
>
>
> On Thu, Sep 17, 2020 at 1:14 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> On 9/16/20 8:01 PM, Han Zhou wrote:
>> > When a lport is referenced by a logical flow where port-binding refs
>> > needs to be added, currently it can add the same reference pair
multiple
>> > times in below situations (introduced in commit ade4e77):
>> >
>> > 1) In add_matches_to_flow_table(), different matches from same lflow
>> >    can have same inport/outport.
>> >
>> > 2) In is_chassis_resident_cb(), a lflow can have multiple
is_chassis_resident
>> >    check for same lport (although not very common), and at the same
time
>> >    the lport used in is_chassis_resident can overlap with the inport/
>> >    outport of the same flow.
>> >
>> > Now because of the redundant entries added, it results in unexpected
behavior
>> > such as same lflow being processed multiple times as a waste of
processing.
>> > More severely, after commit 580aea72e it can result in orphaned
pointer leading
>> > to crash, as reported in [0].
>> >
>> > This patch fixes the problems by checking existence of same reference
before
>> > adding in lflow_resource_add(). To do this check efficiently, hmap is
used to
>> > replace the list struct for the resource-to-lflow index.
>> >
>> > [0]
https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html
>> >
>> > Reported-by: Dumitru Ceara <dceara at redhat.com>
>> > Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html
>> > Fixes: ade4e779d3fb ("ovn-controller: Use the tracked runtime data
changes for flow calculation.")
>> > Fixes: 580aea72e26f ("ovn-controller: Fix conjunction handling with
incremental processing.")
>> > Signed-off-by: Han Zhou <hzhou at ovn.org>
>> > ---
>>
>> Hi Han,
>>
>> Acked-by: Dumitru Ceara <dceara at redhat.com>
>>
>>
>> If possible it would be great if you could also add the detailed
>> comments you shared [1] about how lflow_handle_changed_ref() works
>> before merging it.
>>
>> Otherwise we can add them as a follow up patch, whatever works best for
you.
>
>
> +1 for adding the comment either in this patch or as a separate patch.
>
> Thanks Han for fixing this issue.
>
> Acked-by: Numan Siddique <numans at ovn.org>
>
> Thanks
> Numan
>
Thanks Dumitru and Numan. I applied this to master and backported to branch
20.09 and 20.06.
I didn't add the comments in this patch because it is not related to the
fix itself. I (or anyone) can add it in a separate commit, together with
any refactors if needed.


More information about the dev mailing list