On Mon, Dec 16, 2024 at 9:09 AM Frode Nordahl <fnord...@ubuntu.com> wrote:
>
> On Fri, Dec 13, 2024 at 2:54 PM Eelco Chaudron <echau...@redhat.com> wrote:
> >
> >
> >
> > On 10 Dec 2024, at 23:18, Frode Nordahl wrote:
> >
> > > In preparation for adding support for parsing attributes such as
> > > RTA_VIA and RTA_MULTIPATH, we need dynamic storage for route
> > > nexthops.
> > >
> > > For the common case of one nexthop we store nexthop information in
> > > a static variable to avoid unnecessary memory allocations.
> > >
> > > This patch makes this change in isolation so that it is easier to
> > > review subsequent patches.
> > >
> > > Signed-off-by: Frode Nordahl <fnord...@ubuntu.com>
> >
> > See some comments below.
> >
> > > ---
> > > lib/route-table.c | 63 ++++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 51 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/lib/route-table.c b/lib/route-table.c
> > > index 33e0df2b8..fd26ac688 100644
> > > --- a/lib/route-table.c
> > > +++ b/lib/route-table.c
> > > @@ -32,6 +32,7 @@
> > > #include "netlink.h"
> > > #include "netlink-notifier.h"
> > > #include "netlink-socket.h"
> > > +#include "openvswitch/list.h"
> > > #include "openvswitch/ofpbuf.h"
> > > #include "ovs-router.h"
> > > #include "packets.h"
> > > @@ -47,7 +48,16 @@ VLOG_DEFINE_THIS_MODULE(route_table);
> > >
> > > COVERAGE_DEFINE(route_table_dump);
> > >
> > > +struct route_data_nexthop {
> > > + struct ovs_list nexthop_node;
> > > +
> > > + struct in6_addr addr;
> > > + char ifname[IFNAMSIZ]; /* Interface name. */
> > > +};
> > > +
> > > struct route_data {
> > > + struct ovs_list nexthops;
> > > +
> > > /* Copied from struct rtmsg. */
> > > unsigned char rtm_dst_len;
> > > unsigned char rtm_protocol;
> > > @@ -56,8 +66,6 @@ struct route_data {
> > > /* Extracted from Netlink attributes. */
> > > struct in6_addr rta_dst; /* 0 if missing. */
> > > struct in6_addr rta_prefsrc; /* 0 if missing. */
> > > - struct in6_addr rta_gw;
> > > - char ifname[IFNAMSIZ]; /* Interface name. */
> > > uint32_t mark;
> > > uint32_t rta_table_id; /* 0 if missing. */
> > > uint32_t rta_priority; /* 0 if missing. */
> > > @@ -80,6 +88,7 @@ static uint64_t rt_change_seq;
> > >
> > > static struct nln *nln = NULL;
> > > static struct route_table_msg rtmsg;
> > > +static struct route_data_nexthop rdnh_single;
> > > static struct nln_notifier *route_notifier = NULL;
> > > static struct nln_notifier *route6_notifier = NULL;
> > > static struct nln_notifier *name_notifier = NULL;
> > > @@ -89,12 +98,24 @@ static bool route_table_valid = false;
> > > static void route_table_reset(void);
> > > static void route_table_handle_msg(const struct route_table_msg *);
> > > static int route_table_parse(struct ofpbuf *, void *change);
> > > -static void route_table_change(const struct route_table_msg *, void *);
> > > +static void route_table_change(struct route_table_msg *, void *);
> > > static void route_map_clear(void);
> > >
> > > static void name_table_init(void);
> > > static void name_table_change(const struct rtnetlink_change *, void *);
> > >
> > > +static void
> > > +route_data_destroy(struct route_data *rd)
> > > +{
> > > + struct route_data_nexthop *rdnh;
> > > +
> > > + LIST_FOR_EACH_POP (rdnh, nexthop_node, &rd->nexthops) {
> > > + if (rdnh && rdnh != &rdnh_single) {
> > > + free(rdnh);
> > > + }
> > > + }
> > > +}
> > > +
> > > uint64_t
> > > route_table_get_change_seq(void)
> > > {
> > > @@ -190,6 +211,7 @@ route_table_dump_one_table(unsigned char id)
> > > filtered = false;
> > > }
> > > route_table_handle_msg(&msg);
> > > + route_data_destroy(&msg.rd);
> >
> > If we return an error and have parsed some route entries (patch 6), memory
> > will be lost.
>
> Thanks, the intention was to make sure route_table_parse() would free
> any allocated memory before error return, as there is currently no way
> for the caller to discern between error modes. But it may be I have
> missed a scenario, will revisit.
>
> > > }
> > > }
> > > ofpbuf_uninit(&buf);
> > > @@ -230,6 +252,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > const struct rtmsg *rtm, void *change_)
> > > {
> > > struct route_table_msg *change = change_;
> > > + struct route_data_nexthop *rdnh = NULL;
> > > bool parsed, ipv4 = false;
> > >
> > > static const struct nl_policy policy[] = {
> > > @@ -270,6 +293,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > int rta_oif; /* Output interface index. */
> > >
> > > memset(change, 0, sizeof *change);
> > > + ovs_list_init(&change->rd.nexthops);
> > > + memset(&rdnh_single, 0, sizeof *rdnh);
>
> > Having a global variable does not make this thread-safe. We should have a
> > thread-safe solution for this. I assume you want to avoid allocating memory
> > for the single next-hop case (which is the most common one).
> >
> > I would suggest doing something like this (with some detailed comments,
> > i.e. that the primary entry is private and should not be touched, etc.
> > etc.):
> >
> > struct route_data {
> >
> > + /* LONG COMMENT EXPLAINING ALL OF THE NEXT-HOP STUFF. */
> > struct ovs_list nexthops;
> > + struct route_data_nexthop _primary_next_hop;
> >
> > /* Copied from struct rtmsg. */
> > unsigned char rtm_dst_len;
> > unsigned char rtm_protocol;
> > bool local;
> >
> > /* Extracted from Netlink attributes. */
> > struct in6_addr rta_dst; /* 0 if missing. */
> > struct in6_addr rta_prefsrc; /* 0 if missing. */
> > uint32_t mark;
> > uint32_t rta_table_id; /* 0 if missing. */
> > uint32_t rta_priority; /* 0 if missing. */
> > };
>
> Ack, and thank you for the suggestion. The existing implementation is
> also not thread safe as it uses a static variable for storage of
> struct route_table_msg that holds struct route_data, so I only
> continued the same mode of operation.
>
> But if an external consumer wants to make use of threads, I guess it
> would be much better if we were to allow that. Storing this in a
> different way and adding a once per thread initializer would not be
> that hard, so I'll make an attempt at doing that as part of this
> series.
Looking over this again, the existing static storage is only used for
the netlink-notifier callback, which I think makes sense to keep
as-is.
I think I was fooled by the variable naming though, so I'll propose
something that makes it more clear.
External consumers are expected to set up their own netlink-notifier
as needed, so I'll also add some more documentation around the two use
cases for the module, i.e. internal OVS use together with ovs-router
vs. external consumption of route dump/parse functions.
--
Frode Nordahl
> --
> Frode Nordahl
>
> > > + rdnh = &rdnh_single;
> > > change->relevant = true;
> > >
> > > if (rtm->rtm_scope == RT_SCOPE_NOWHERE) {
> > > @@ -293,7 +319,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > if (attrs[RTA_OIF]) {
> > > rta_oif = nl_attr_get_u32(attrs[RTA_OIF]);
> > >
> > > - if (!if_indextoname(rta_oif, change->rd.ifname)) {
> > > + if (!if_indextoname(rta_oif, rdnh->ifname)) {
> > > int error = errno;
> > >
> > > VLOG_DBG_RL(&rl, "Could not find interface name[%u]: %s",
> > > @@ -301,7 +327,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > if (error == ENXIO) {
> > > change->relevant = false;
> > > } else {
> > > - return 0;
> > > + goto error_out;
> > > }
> > > }
> > > }
> > > @@ -331,9 +357,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > if (ipv4) {
> > > ovs_be32 gw;
> > > gw = nl_attr_get_be32(attrs[RTA_GATEWAY]);
> > > - in6_addr_set_mapped_ipv4(&change->rd.rta_gw, gw);
> > > + in6_addr_set_mapped_ipv4(&rdnh->addr, gw);
> > > } else {
> > > - change->rd.rta_gw =
> > > nl_attr_get_in6_addr(attrs[RTA_GATEWAY]);
> > > + rdnh->addr = nl_attr_get_in6_addr(attrs[RTA_GATEWAY]);
> > > }
> > > }
> > > if (attrs[RTA_MARK]) {
> > > @@ -342,13 +368,20 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > > if (attrs[RTA_PRIORITY]) {
> > > change->rd.rta_priority =
> > > nl_attr_get_u32(attrs[RTA_PRIORITY]);
> > > }
> > > + ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
> >
> > I think we need to add some proper error handling if there is no RTA_VIA or
> > RTA_GATEWAY, we should error out as the message is not valid.
> >
> > > } else {
> > > VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message");
> > > - return 0;
> > > + goto error_out;
> > > }
> > >
> > > /* Success. */
> > > return ipv4 ? RTNLGRP_IPV4_ROUTE : RTNLGRP_IPV6_ROUTE;
> > > +
> > > +error_out:
> > > + if (rdnh && rdnh != &rdnh_single) {
> > > + free (rdnh);
> > > + }
> > > + return 0;
> > > }
> > >
> > > static int
> > > @@ -374,7 +407,7 @@ route_table_standard_table(uint32_t table_id)
> > > }
> > >
> > > static void
> > > -route_table_change(const struct route_table_msg *change OVS_UNUSED,
> > > +route_table_change(struct route_table_msg *change,
> > > void *aux OVS_UNUSED)
> >
> > This will now fit on one line.
> >
> > > {
> > > if (!change
> > > @@ -382,6 +415,9 @@ route_table_change(const struct route_table_msg
> > > *change OVS_UNUSED,
> > > && route_table_standard_table(change->rd.rta_table_id))) {
> > > route_table_valid = false;
> > > }
> > > + if (change && !ovs_list_is_empty(&change->rd.nexthops)) {
> >
> > Maybe this ovs_list_is_empty should be skipped as these details should be
> > dealt with in route_data_destroy().
> >
> > > + route_data_destroy(&change->rd);
> > > + }
> > > }
> > >
> > > static void
> > > @@ -389,10 +425,13 @@ route_table_handle_msg(const struct route_table_msg
> > > *change)
> > > {
> > > if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) {
> > > const struct route_data *rd = &change->rd;
> > > + const struct route_data_nexthop *rdnh;
> > >
> > > - ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
> > > - rd->local, rd->ifname, &rd->rta_gw,
> > > - &rd->rta_prefsrc);
> > > + LIST_FOR_EACH (rdnh, nexthop_node, &rd->nexthops) {
> > > + ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
> > > + rd->local, rdnh->ifname, &rdnh->addr,
> > > + &rd->rta_prefsrc);
> > > + }
> > > }
> > > }
> > >
> > > --
> > > 2.45.2
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
