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.
> > > > >
> > > > > 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
