numan
On Thu, Sep 25, 2025, 2:47 PM Ales Musil <amu...@redhat.com> wrote:
>
>
> On Thu, Sep 25, 2025 at 7:27 PM Mark Michelson <mmich...@redhat.com>
> wrote:
>
>> On Thu, Sep 25, 2025 at 1:23 PM Mark Michelson <mmich...@redhat.com>
>> wrote:
>> >
>> > On Thu, Sep 25, 2025 at 1:21 PM Mark Michelson <mmich...@redhat.com>
>> wrote:
>> > >
>> > > On Thu, Sep 25, 2025 at 1:13 PM Mark Michelson <mmich...@redhat.com>
>> wrote:
>> > > >
>> > > > I can see the reasoning behind the current code. If the port is
>> > > > definitely not associated with a router at all, then don't bother
>> > > > trying to look in lr_ports for it. It saves a lookup into lr_ports.
>> > > > However, I wonder in practice how much computation this actually
>> > > > saves. get_lport_type() is an if-else ladder that performs strcmp()
>> > > > operations. ovn_port_find() does a hashed lookup of the port and a
>> > > > strcmp() to verify that the hashed port is the one we want. If the
>> > > > port does not exist in lr_ports at all, there is a decent chance
>> that
>> > > > HMAP_FOR_EACH_WITH_HASH () will calculate the hash, provide no
>> > > > matches, and end the loop after zero iterations.
>> > > >
>> > > > Without having measured this at all (IMPORTANT!!), I would reason
>> that
>> > > > doing two ovn_port_find() operations in lr_ports and then ls_ports
>> > > > might be cheaper than calling get_lport_type() and then calling
>> > > > ovn_port_find() in ls_ports. And it certainly is cheaper than
>> calling
>> > > > get_lport_type(), and then calling ovn_port_find() two times.
>> > > >
>> > > > If we really need to measure this, then we can, but I currently am
>> in
>> > > > support of the change proposed by Ales. There is a chance this
>> > > > actually improves performance compared to the current code, and it
>> > > > eliminates a function whose name can be misleading.
>> > >
>> > > And one final note: If the goal is simply to tell if a port_binding is
>> > > associated with a switch or router, then keep in mind that in 25.09+,
>> > > the datapath_binding table has a "type" field on it. An alternate idea
>> > > would be to redefine is_pb_router_type() in the following way:
>> > >
>> > > bool
>> > > is_pb_router_type(const struct port_binding *pb)
>> > > {
>> > > if (!pb->datapath) {
>> > > return false;
>> > > }
>> > > if (!strcmp(pb->datapath->type, "logical-router")) {
>> > > return true;
>> > > }
>> > > return false;
>> > > }
>> >
>> > And of course immediately after I sent this I realized this can be a
>> one-liner:
>> >
>> > return pb->datapath && !stcmp(pb->datapath->type, "logical_router");
>> >
>>
>> Last message from me :)
>>
>> Instead of looking directly into pb->datapath->type, it is safer to do:
>>
>> return pb->datapath && !strcmp(datapath_get_nb_type(pb->datapath),
>> "logical-router");
>>
>
> Thank you Mark, that is a better check indeed.
> Looking at the schema the pb->datapath cannot be null:
>
> [SBREC_PORT_BINDING_COL_DATAPATH] = {
> .name = "datapath",
> .type = {
> .key = {
> .type = OVSDB_TYPE_UUID,
> .uuid = { .refTableName = "Datapath_Binding", .refType =
> OVSDB_REF_STRONG },
> },
> .value = OVSDB_BASE_VOID_INIT,
> .n_min = 1,
> .n_max = 1,
> },
> .is_mutable = true,
> .is_synthetic = false,
> .parse = sbrec_port_binding_parse_datapath,
> .unparse = sbrec_port_binding_unparse_datapath,
> },
>
> So I have removed the pb->datapath check from the suggestion in v2.
>
>
>>
>> > >
>> > > Then you have a relatively cheap function that can take a port binding
>> > > and determine for certain whether it is associated with a logical
>> > > router. This would allow for us to always perform a single strcmp()
>> > > and a single ovn_port_find() call.
>> > >
>> > > >
>> > > > On Thu, Sep 25, 2025 at 10:48 AM Numan Siddique <num...@ovn.org>
>> wrote:
>> > > > >
>> > > > > On Thu, Sep 25, 2025 at 8:21 AM Ales Musil via dev
>> > > > > <ovs-dev@openvswitch.org> wrote:
>> > > > > >
>> > > > > > That function is misleading as the type isn't guaranteed to be
>> > > > > > always LRP. The only place where this function was used knew
>> about
>> > > > > > this hidden logic and did a lookup the lookup twice anyway.
>> Let's
>> > > > > > skip this micro optimization whatsoever and do the lookup twice
>> > > > > > when needed.
>> > > > > >
>> > > > > > Signed-off-by: Ales Musil <amu...@redhat.com>
>> > > > >
>> > > > > I agree with the function name not being very intuitive. When I
>> added
>> > > > > this function, I meant a port binding
>> > > > > which is of type router. If you see, we have a logical switch
>> port of
>> > > > > type "router". This function doesn't
>> > > > > say if the corresponding logical port of the "port_binding" in
>> the NB
>> > > > > DB is a _logical_router_port_ or not, but it indicates
>> > > > > if it is a router port (which can belong to a logical switch or
>> router).
>> > > > >
>> > > > > With this patch, whenever a port binding (irrespective of type)
>> gets
>> > > > > updated we will lookup the lr_map first and then ls_map
>> > > > > and we are introducing an extra cost of look up. Which is not the
>> > > > > case without this patch. For VIF port bindings we will only
>> lookup in
>> > > > > the ls_map.
>> > > > >
>> > > > > If we don't want to use this function, my suggestion would be for
>> > > > > northd to write "datapath=router" or "datapath=switch" in the
>> > > > > external_ids of port bindings.
>> > > > > This way we can do a lookup in the proper map when handling port
>> > > > > binding changes.
>>
>
>
> I personally think that the double lookup will not cost us any
> performance whatsoever. To be on the safe side I have slightly
> adjusted the suggestion from Mark. We should be able to tell
> from the datapath right away if it's LSP or LRP. So we will never
> do double lookup even for overlapping types (peer, remote etc.).
> I'll send v2 with this change.
>
Sounds good to me.
Thanks
>
>> > > > >
>> > > > > Thanks
>> > > > > Numan
>> > > > >
>> > > > >
>> > > > > > ---
>> > > > > > lib/ovn-util.c | 28 ----------------------------
>> > > > > > lib/ovn-util.h | 1 -
>> > > > > > northd/northd.c | 24 +++++++++---------------
>> > > > > > 3 files changed, 9 insertions(+), 44 deletions(-)
>> > > > > >
>> > > > > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>> > > > > > index 6c7629e6d..7b77f838f 100644
>> > > > > > --- a/lib/ovn-util.c
>> > > > > > +++ b/lib/ovn-util.c
>> > > > > > @@ -1283,34 +1283,6 @@ get_lport_type_str(enum en_lport_type
>> lport_type)
>> > > > > > OVS_NOT_REACHED();
>> > > > > > }
>> > > > > >
>> > > > > > -bool
>> > > > > > -is_pb_router_type(const struct sbrec_port_binding *pb)
>> > > > > > -{
>> > > > > > - enum en_lport_type lport_type = get_lport_type(pb);
>> > > > > > -
>> > > > > > - switch (lport_type) {
>> > > > > > - case LP_PATCH:
>> > > > > > - case LP_CHASSISREDIRECT:
>> > > > > > - case LP_L3GATEWAY:
>> > > > > > - case LP_L2GATEWAY:
>> > > > > > - case LP_REMOTE:
>> > > > > > - return true;
>> > > > > > -
>> > > > > > - case LP_VIF:
>> > > > > > - case LP_CONTAINER:
>> > > > > > - case LP_MIRROR:
>> > > > > > - case LP_VIRTUAL:
>> > > > > > - case LP_LOCALNET:
>> > > > > > - case LP_LOCALPORT:
>> > > > > > - case LP_VTEP:
>> > > > > > - case LP_EXTERNAL:
>> > > > > > - case LP_UNKNOWN:
>> > > > > > - return false;
>> > > > > > - }
>> > > > > > -
>> > > > > > - return false;
>> > > > > > -}
>> > > > > > -
>> > > > > > void
>> > > > > > sorted_array_apply_diff(const struct sorted_array *a1,
>> > > > > > const struct sorted_array *a2,
>> > > > > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>> > > > > > index 67fc3882d..4c686aac9 100644
>> > > > > > --- a/lib/ovn-util.h
>> > > > > > +++ b/lib/ovn-util.h
>> > > > > > @@ -433,7 +433,6 @@ enum en_lport_type {
>> > > > > >
>> > > > > > enum en_lport_type get_lport_type(const struct
>> sbrec_port_binding *);
>> > > > > > char *get_lport_type_str(enum en_lport_type lport_type);
>> > > > > > -bool is_pb_router_type(const struct sbrec_port_binding *);
>> > > > > >
>> > > > > > /* A wrapper that holds sorted arrays of strings. */
>> > > > > > struct sorted_array {
>> > > > > > diff --git a/northd/northd.c b/northd/northd.c
>> > > > > > index fe5199a86..e1d01df50 100644
>> > > > > > --- a/northd/northd.c
>> > > > > > +++ b/northd/northd.c
>> > > > > > @@ -4982,23 +4982,17 @@ northd_handle_sb_port_binding_changes(
>> > > > > > const struct sbrec_port_binding *pb;
>> > > > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1,
>> 5);
>> > > > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
>> sbrec_port_binding_table) {
>> > > > > > - bool is_router_port = is_pb_router_type(pb);
>> > > > > > - struct ovn_port *op = NULL;
>> > > > > > -
>> > > > > > - if (is_router_port) {
>> > > > > > - /* A router port binding 'pb' can belong to
>> > > > > > - * - a logical switch port of type router or
>> > > > > > - * - a logical router port.
>> > > > > > - *
>> > > > > > - * So, first search in lr_ports hmap. If not
>> found, search in
>> > > > > > - * ls_ports hmap.
>> > > > > > - * */
>> > > > > > - op = ovn_port_find(lr_ports, pb->logical_port);
>> > > > > > - }
>> > > > > > -
>> > > > > > + bool is_router_port = true;
>> > > > > > + /* A router port binding 'pb' can belong to
>> > > > > > + * - a logical switch port of type router or
>> > > > > > + * - a logical router port.
>> > > > > > + *
>> > > > > > + * So, first search in lr_ports hmap. If not found,
>> search in
>> > > > > > + * ls_ports hmap.
>> > > > > > + * */
>> > > > > > + struct ovn_port *op = ovn_port_find(lr_ports,
>> pb->logical_port);
>> > > > > > if (!op) {
>> > > > > > op = ovn_port_find(ls_ports, pb->logical_port);
>> > > > > > -
>> > > > > > if (op) {
>> > > > > > is_router_port = false;
>> > > > > > }
>> > > > > > --
>> > > > > > 2.51.0
>> > > > > >
>> > > > > > _______________________________________________
>> > > > > > dev mailing list
>> > > > > > d...@openvswitch.org
>> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > > > > _______________________________________________
>> > > > > dev mailing list
>> > > > > d...@openvswitch.org
>> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> Thanks,
> Ales
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
