On Fri, Jan 3, 2025 at 5:59 PM Eelco Chaudron <[email protected]> wrote:
>
> On 18 Dec 2024, at 20:12, Frode Nordahl wrote:
>
> > In preparation for adding support for parsing routes with multiple
> > next hops per destination, we need dynamic storage for them.
> >
> > Storage for the primary next hop is set aside in struct route_data
> > to avoid unnecessary memory allocations for the common case of one
> > next hop.
> >
> > The ovs-router module does not support storing multiple next hop
> > information, and the existing parser does not understand them, so
> > we retain this behavior by only passing single next hop routes for
> > storage in ovs-router.
> >
> > This patch makes this change in isolation so that it is easier to
> > review subsequent patches.
> >
> > Signed-off-by: Frode Nordahl <[email protected]>
>
> See some comments below.
>
> //Eelco
>
> > ---
> > lib/route-table.c | 94 ++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 81 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/route-table.c b/lib/route-table.c
> > index 4e8f35199..b6f0223c0 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,34 @@ 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 {
> > + /* Routes can have multiple next hops per destination.
> > + *
> > + * Each next hop has its own set of attributes such as address family,
> > + * interface and IP address.
> > + *
> > + * When retrieving information about a route from the kernel, in the
> > case
> > + * of multiple next hops, information is provided as nested attributes.
> > + *
> > + * A linked list with struct route_data_nexthop entries is used to
> > store
> > + * this information as we parse each attribute.
> > + *
> > + * For the common case of one next hop, the nexthops list will contain
> > a
> > + * single entry pointing to the struct route_data _primary_next_hop
> > + * element.
> > + *
> > + * Any dynamically allocated list elements can be freed with a call to
> > the
>
> Maybe this should be more strict, i.e. 'MUST be freed with a ....'
Done.
> > + * route_data_destroy function. */
> > + struct ovs_list nexthops;
> > + struct route_data_nexthop _primary_next_hop;
> > +
> > /* Copied from struct rtmsg. */
> > unsigned char rtm_dst_len;
> > unsigned char rtm_protocol;
> > @@ -56,8 +84,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. */
> > @@ -89,12 +115,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 *aux);
> > 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 != &rd->_primary_next_hop) {
> > + free(rdnh);
> > + }
> > + }
> > +}
> > +
> > uint64_t
> > route_table_get_change_seq(void)
> > {
> > @@ -190,6 +228,7 @@ route_table_dump_one_table(unsigned char id)
> > filtered = false;
> > }
> > route_table_handle_msg(&msg);
> > + route_data_destroy(&msg.rd);
> > }
> > }
> > ofpbuf_uninit(&buf);
> > @@ -222,13 +261,12 @@ route_table_reset(void)
> > }
> > }
> >
> > -/* Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on parse
> > - * error. */
> > static int
> > route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > const struct nlmsghdr *nlmsg,
> > const struct rtmsg *rtm, struct route_table_msg
> > *change)
> > {
> > + struct route_data_nexthop *rdnh = NULL;
>
> This can be defined lower in the if statement and moved in later patches if
> required.
Indeed, originally it was referenced in the out: label, which is why
it was declared here initially.
> > bool parsed, ipv4 = false;
> >
> > static const struct nl_policy policy[] = {
> > @@ -269,6 +307,12 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > int rta_oif; /* Output interface index. */
> >
> > memset(change, 0, sizeof *change);
> > +
> > + /* ovs_list_init / ovs_list_insert does not allocate any memory */
>
> Don't think this comment is necessary. If you do think it is, change it to
> something like:
I'll just drop it, I added it on the back of an earlier review comment
about potential memory leak when I found none.
I think the documentation added for route_table_parse() covers the
memory ownership and behavior adequately.
> /* ovs_list_init() and ovs_list_insert() do not allocate any memory.
> */
>
> > + ovs_list_init(&change->rd.nexthops);
> > + rdnh = &change->rd._primary_next_hop;
> > + ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
> > +
> > change->relevant = true;
> >
> > if (rtm->rtm_scope == RT_SCOPE_NOWHERE) {
> > @@ -292,7 +336,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",
> > @@ -300,7 +344,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > if (error == ENXIO) {
> > change->relevant = false;
>
> Now that this becomes an external API, the content might still be used even
> if relevant was set to false. So maybe it will be good to initalize the
> string in the error case.
>
> rdnh->ifname[0] = 0;
In the case of single next hop the memset of *change will take care of
this, in the case of multiple next hops that come in a later patch the
use of xzalloc() will take care of this. Or did I miss the intention
of your comment?
>
> > } else {
> > - return 0;
> > + goto error_out;
> > }
> > }
> > }
> > @@ -330,9 +374,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]) {
> > @@ -343,13 +387,27 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > }
> > } 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:
> > + route_data_destroy(&change->rd);
> > + return 0;
> > }
> >
> > +/* Parse Netlink message in buf, which is expected to contain UAPI rtmsg
> > + * header and associated route attributes.
> > + *
> > + * Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on parse
> > + * error.
> > + *
> > + * On success, memory may have be allocated, and it is the callers
> > + * responsibility to free it with a call to route_data_destroy.
> > + *
> > + * In case of error, any allocated memory will be freed before return. */
>
> Some spelling corrections, highlighted with **
>
> /* Parse Netlink message in buf, which is expected to contain **a** UAPI rtmsg
> * header and associated route attributes.
> *
> * Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, **and** 0 on
> **a** parse
> * error.
> *
> * On success, memory may have **been** allocated, and it is the **caller’s**
> * responsibility to free it with a call to route_data_destroy**()**.
> *
> * In case of error, any allocated memory will be freed before **returning**.
> */
Done, thanks!
> > static int
> > route_table_parse(struct ofpbuf *buf, void *change)
> > {
> > @@ -373,7 +431,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)
>
> Same comment as before; This will now fit on one line.
Done.
> > {
> > if (!change
> > @@ -381,16 +439,26 @@ 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) {
> > + route_data_destroy(&change->rd);
> > + }
> > }
> >
> > static void
> > route_table_handle_msg(const struct route_table_msg *change)
> > {
> > - if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) {
> > + if (change->relevant && change->nlmsg_type == RTM_NEWROUTE
> > + && ovs_list_is_singleton(&change->rd.nexthops)) {
>
> Based on your own comment, I would change it here to
> !ovs_list_is_empty(&change->rd.nexthops) in this patch already.
Done.
> > const struct route_data *rd = &change->rd;
> > + const struct route_data_nexthop *rdnh;
> > +
> > + /* The ovs-router module does currently not implement lookup nor
> > + * storage for routes with multiple next hops. */
>
> Maybe enhance the comment with:
>
> /* The ovs-router module currently does not implement lookup or
> * storage for routes with multiple next hops. For backwards
> * compatibility, we use the first next hop. */
Thanks, done.
--
Frode Nordahl
> > + rdnh = CONTAINER_OF(ovs_list_front(&change->rd.nexthops),
> > + const struct route_data_nexthop, nexthop_node);
> >
> > ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
> > - rd->local, rd->ifname, &rd->rta_gw,
> > + rd->local, rdnh->ifname, &rdnh->addr,
> > &rd->rta_prefsrc);
> > }
> > }
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
