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;
}
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.
> >
> > 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
