On 6/28/21 8:05 AM, Han Zhou wrote:
> On Fri, Jun 25, 2021 at 2:38 PM Numan Siddique <num...@ovn.org> wrote:
>>
>> On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <hz...@ovn.org> wrote:
>>>
>>> On Fri, Jun 25, 2021 at 4:50 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>>>
>>>> It's valid that port_groups contain non-vif ports, they can actually
>>>> contain any type of logical_switch_port.
>>>>
>>>> Also, there's no need to allocate a new sset containing the local
> ports'
>>>> names every time the I-P engine processes a change, we can maintain a
>>>> sset and incrementally update it when port bindings are added/removed.
>>>>
>>> Thanks Dumitru for the fix and thanks Numan for the review.
>>>
>>> I battled with myself when deciding to allocate a new sset for the local
>>> ports' names at the I-P main iteration level. I did this because the
>>> current data structures maintaining the local bindings were already very
>>> complex, and the sset was not to maintain any extra information but just
>>> redundant information (for efficiency). So I decided to abstract this
> part
>>> as a helper function so that I don't add more complexity in the binding
>>> data structure, and other I-P engine nodes doesn't need to understand
> the
>>> internals of how the bindings are maintained in the bindings module.
>>> Regarding the cost, the local binding data should be small, and the sset
>>> allocation is at the main loop level, so really nothing to worry about
> the
>>> cost.
>>>
>>> However, I didn't think about the non-VIF use case of port-group, and
> the
>>> local_binding doesn't maintain non-VIF bindings, so came the bug. This
>>> patch fixes it by maintaining a sset that includes all types of lport
>>> names. It looks correct to me, but I have some comments:
>>>
>>> 1) The structures in bindings module is really complex and I spent a
> lot of
>>> time to understand it before, but when I am reviewing this patch I had
> to
>>> spent some time again to understand it. There are fields in binding_ctx
>>> look quite similar, and the comments don't tell exactly the difference:
>>>
>>> - struct local_binding_data *lbinding_data;
>>> - struct sset *local_lports;
>>> - struct sset *local_lport_ids;
>>>
>>
>> I agree with the complexity and the naming confusion.
>>
>> I think local_lports and local_lport_ids have been maintained in the
>> binding code
>> since a long time and lbinding_data was added recently.
>>
>> I think there is a lot of redundant data which can be unified.
>>
>>
>>> According to the code (and also the bug the patch is trying to fix),
>>> lbinding_data is supposed to maintain VIFs only.
>>
>> I agree. "lbinding_data" is supposed to maintain local vif binding
> information.
>>
>>> local_lports maintains all types, but it maintains *potentially* local
> VIFs
>>> as well (meaning the VIF may not be bound locally yet). I was thinking
> if I
>>> could use local_lports directly. I think it would work, but just not
>>> accurate enough (maybe it doesn't really matter).
>>
>>
>>> The local_lport_ids may look straightforward, which maintains generated
> id
>>> keys for local_lports, but the functions update_local_lport_ids() and
>>> remove_local_lport_ids() are not only updating the local_lport_ids, but
>>> also tracking information of lbinding_data, which is really confusing.
>>>
>>> 2) Now for this patch, the intention is to include non-VIF bindings,
> but it
>>> adds a sset to maintain all types of lports in "lbinding_data", which
> was
>>> supposed to maintain VIF bindings only. I think it is not the right
> place
>>> to maintain this sset. And the
>>> update_local_lport_ids()/remove_local_lport_ids() are not the right
> place
>>> to update them either.
>>>
>>> So here are my suggestions:
>>>
>>> 1) Clarify a little more about the role of each of the above fields in
>>> binding_ctx with some comments.
>>
>> These comments would be super helpful. But I think it is outside the
>> scope of this bug fix patch. It's better if it's a separate patch.
>
> Agree. And I just noticed that the comments for local_lport_ids in
> ovn-controller.c is not correct any more (probably since very long time
> ago):
> /* Contains the same ports as local_lports, but in the format:
>
> * <datapath-tunnel-key>_<port-tunnel-key> */
>
> struct sset local_lport_ids;
>
> It in fact contains more lports than local_lports, including patch ports,
> and they are used for different purposes. I think we should rename them.
>
>>
>>> 2) Can we use local_lports directly, instead of maintaining a new sset?
> If
>>> we can't (I am not sure yet), can we generate it on-the-fly, just
> updating
>>> the "binding_collect_local_binding_lports" by adding non-VIF lports from
>>> "local_lports"? I really don't think the cost makes any difference
> overall.
>>> If none of the above work, can we maintain it as a separate field at
>>> binding_ctx level instead of in lbinding_data (with proper comment
> telling
>>> the difference from local_lports)?
>>
>> I think local_lports can be used. The side effect would be that we
>> will be allocating
>> the zone id even for patch ports.
>>
> Thanks Numan. I checked the code again and realized that local_lports
> doesn't contain patch ports, so it cannot be used for the port-group I-P
> directly. I think you meant that it can be reused if we add patch ports to
> it, but then zone id allocation behavior will be affected. In my opinion
> we'd better not do so because they serve different purposes. It looks that
> what is required by port-group processing is close to what we have for
> local_lport_ids, but just names instead of tunnel keys, so we may combine
> with local_lport_ids as a struct, e.g.:
>
> struct related_lports {
> struct sset lport_names;
> struct sset lport_ids; /* the current local_lport_ids */
> }
>
> "related_lports" represents lports that are related to this chassis,
> including patch ports, to distinguish from the "local_lports". Maybe the
> name is still not good, but I just couldn't think of a better one.
> The functions update_local_lport_ids()/remove_local_lport_ids() are indeed
> the right place to update it (except that we should rename them).
>
> Thanks,
> Han
>
Thanks Han and Numan for the reviews and discussion. I went ahead and
sent v2 with the "related_lports" structure suggested above:
http://patchwork.ozlabs.org/project/ovn/list/?series=251337&state=*
I added comments to the new structure but indeed we likely need a follow
up patch to document more of the remaining structures in binding.h.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
