>> This both bumps the OVS submodule and fixes various breaking changes >> introduced in OVS version 3.7.0. >> >> - OVS commit dc14e92 updates route_table_handle_msg_callback to have a >> 3rd parameter 'uint32_t table', which allows us to remove table_id from >> the route_msg_handle_data struct provided we update handle_route_msg >> accordingly. >> - OVS commit 3900653 changes FLOW_N_REGS from 16 to 32, to avoid version >> incompatibility errors we set OVN_FLOW_N_REGS_SUPPORTED which is 16, >> and is checked to be within FLOW_N_REGS. We then check that the >> various MFF_LOG declarations fall within this still. >> >> This should allow us to sync up with OVS again in preparation for 26.03. >> >> Signed-off-by: MJ Ponsonby <mj.ponsonby at canonical.com> >> --- >> controller/pinctrl.c | 2 +- >> controller/route-exchange-netlink.c | 9 +++---- >> include/ovn/logical-fields.h | 39 +++++++++++++++++++++++++++++ >> lib/actions.c | 4 +-- >> ovs | 2 +- >> utilities/ovn-trace.c | 2 +- >> 6 files changed, 48 insertions(+), 10 deletions(-) > > Thanks, MJ! This addresses all the issues and seems to work fine. > > Just in case, I also ran OVN unit tests with OVS 3.5 binaries while > OVN was built with 3.7. And it worked fine with no issues. > > See a couple of small comments below. >
Hi, Ilya Maximets. Thanks for the quick response, I'm promptly posting a v2 to address your comments. >> #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway >> router >> * (32 bits). */ >> +CHECK_REG(DNAT_ZONE); >> #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway >> router >> * (32 bits). */ >> +CHECK_REG(SNAT_ZONE); >> #define MFF_LOG_CT_ZONE MFF_REG13 /* Logical conntrack zone for lports >> * (0..15 of the 32 bits). */ >> +CHECK_REG(CT_ZONE); >> #define MFF_LOG_ENCAP_ID MFF_REG13 /* Encap ID for lports >> * (16..31 of the 32 bits). */ >> +CHECK_REG(ENCAP_ID); >> #define MFF_LOG_INPORT MFF_REG14 /* Logical input port (32 bits). */ >> +CHECK_REG(INPORT); >> #define MFF_LOG_OUTPORT MFF_REG15 /* Logical output port (32 bits). */ >> +CHECK_REG(OUTPORT); >> #define MFF_LOG_TUN_OFPORT MFF_REG5 /* 16..31 of the 32 bits */ >> +CHECK_REG(TUN_OFPORT); >> >> /* Logical registers. >> * >> * Make sure these don't overlap with the logical fields! */ >> #define MFF_LOG_REG0 MFF_REG0 >> +CHECK_REG(REG0); >> #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG4 >> +CHECK_REG(LB_ORIG_DIP_IPV4); >> #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 >> +CHECK_REG(LB_ORIG_TP_DPORT); >> >> #define MFF_LOG_XXREG0 MFF_XXREG0 > > Missed this one. I felt this one was okay to miss as it is built around MFF_XXREG0 which is always going to be within the range, however its no harm to add it in the v2. > Best regards, Ilya Maximets. Kind regards, MJ Ponsonby. On Tue, 10 Feb 2026 at 00:16, Ilya Maximets <[email protected]> wrote: > > On 2/9/26 6:54 PM, MJ Ponsonby wrote: > > This both bumps the OVS submodule and fixes various breaking changes > > introduced in OVS version 3.7.0. > > > > - OVS commit dc14e92 updates route_table_handle_msg_callback to have a > > 3rd parameter 'uint32_t table', which allows us to remove table_id from > > the route_msg_handle_data struct provided we update handle_route_msg > > accordingly. > > - OVS commit 3900653 changes FLOW_N_REGS from 16 to 32, to avoid version > > incompatibility errors we set OVN_FLOW_N_REGS_SUPPORTED which is 16, > > and is checked to be within FLOW_N_REGS. We then check that the > > various MFF_LOG declarations fall within this still. > > > > This should allow us to sync up with OVS again in preparation for 26.03. > > > > Signed-off-by: MJ Ponsonby <[email protected]> > > --- > > controller/pinctrl.c | 2 +- > > controller/route-exchange-netlink.c | 9 +++---- > > include/ovn/logical-fields.h | 39 +++++++++++++++++++++++++++++ > > lib/actions.c | 4 +-- > > ovs | 2 +- > > utilities/ovn-trace.c | 2 +- > > 6 files changed, 48 insertions(+), 10 deletions(-) > > Thanks, MJ! This addresses all the issues and seems to work fine. > > Just in case, I also ran OVN unit tests with OVS 3.5 binaries while > OVN was built with 3.7. And it worked fine with no issues. > > See a couple of small comments below. > > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 7baf6b488..08940cd8b 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -6368,7 +6368,7 @@ static void > > reload_metadata(struct ofpbuf *ofpacts, const struct match *md) > > { > > enum mf_field_id md_fields[] = { > > -#if FLOW_N_REGS == 16 > > +#if OVN_FLOW_N_REGS_SUPPORTED == 16 > > MFF_REG0, > > MFF_REG1, > > MFF_REG2, > > diff --git a/controller/route-exchange-netlink.c > > b/controller/route-exchange-netlink.c > > index 7ab0690f2..1fb41008c 100644 > > --- a/controller/route-exchange-netlink.c > > +++ b/controller/route-exchange-netlink.c > > @@ -205,17 +205,18 @@ struct route_msg_handle_data { > > struct vector *learned_routes; > > struct vector *stale_routes; > > const struct hmap *routes; > > - uint32_t table_id; /* requested table id. */ > > }; > > > > static void > > -handle_route_msg(const struct route_table_msg *msg, void *data) > > +handle_route_msg(const struct route_table_msg *msg, > > + void *data, > > + uint32_t table_id) > > { > > struct route_msg_handle_data *handle_data = data; > > const struct route_data *rd = &msg->rd; > > struct advertise_route_entry *ar; > > > > - if (handle_data->table_id != rd->rta_table_id) { > > + if (table_id != rd->rta_table_id) { > > /* We do not have the NLM_F_DUMP_FILTERED info here, so check if > > the > > * reported table_id matches the requested one. > > */ > > @@ -342,7 +343,6 @@ re_nl_sync_routes(uint32_t table_id, const struct hmap > > *routes, > > .learned_routes = learned_routes, > > .stale_routes = &stale_routes, > > .db = db, > > - .table_id = table_id, > > }; > > route_table_dump_one_table(table_id, handle_route_msg, &data); > > > > @@ -388,7 +388,6 @@ re_nl_cleanup_routes(uint32_t table_id) > > .routes_to_advertise = NULL, > > .learned_routes = NULL, > > .stale_routes = &stale_routes, > > - .table_id = table_id, > > }; > > route_table_dump_one_table(table_id, handle_route_msg, &data); > > > > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > > index f0d34196a..fa96cc3c9 100644 > > --- a/include/ovn/logical-fields.h > > +++ b/include/ovn/logical-fields.h > > @@ -26,53 +26,92 @@ enum ovn_controller_event { > > OVN_EVENT_MAX, > > }; > > > > +/* OVS versions 2.6 to 3.6 support up to 16 32-bit registers. These can be > > + * used unconditionally. Registers above MFF_REG15, MFF_XREG7 and > > MFF_XXREG3 > > + * are only supported in newer versions and must not be used without prior > > + * support detection. */ > > +#define OVN_FLOW_N_REGS_SUPPORTED 16 > > +BUILD_ASSERT_DECL(FLOW_N_REGS == 32); > > +BUILD_ASSERT_DECL(FLOW_N_REGS >= OVN_FLOW_N_REGS_SUPPORTED); > > + > > +#define CHECK_REG(NAME) \ > > + BUILD_ASSERT_DECL( \ > > + MFF_LOG_##NAME < MFF_REG0 + OVN_FLOW_N_REGS_SUPPORTED) > > + > > +#define CHECK_XXREG(NAME) \ > > + BUILD_ASSERT_DECL( \ > > + MFF_LOG_##NAME < \ > > + MFF_XXREG0 + OVN_FLOW_N_REGS_SUPPORTED / 4) > > nit: The last two lines can be one. There is enough space. > > > + > > + > > /* Logical fields. > > * > > * These values are documented in ovn-architecture(7), please update the > > * documentation if you change any of them. */ > > #define MFF_LOG_DATAPATH MFF_METADATA /* Logical datapath (64 bits). */ > > #define MFF_LOG_FLAGS MFF_REG10 /* One of MLF_* (32 bits). */ > > +CHECK_REG(FLAGS); > > Note: > I debated with myself if it's better to check right after every > definition or all together as a single block afterwards. And it > seems like having checks right after each definition is indeed > better, as it will be harder to forget to add a check if a new > definition is added. > > > #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway > > router > > * (32 bits). */ > > +CHECK_REG(DNAT_ZONE); > > #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway > > router > > * (32 bits). */ > > +CHECK_REG(SNAT_ZONE); > > #define MFF_LOG_CT_ZONE MFF_REG13 /* Logical conntrack zone for lports > > * (0..15 of the 32 bits). */ > > +CHECK_REG(CT_ZONE); > > #define MFF_LOG_ENCAP_ID MFF_REG13 /* Encap ID for lports > > * (16..31 of the 32 bits). */ > > +CHECK_REG(ENCAP_ID); > > #define MFF_LOG_INPORT MFF_REG14 /* Logical input port (32 bits). */ > > +CHECK_REG(INPORT); > > #define MFF_LOG_OUTPORT MFF_REG15 /* Logical output port (32 bits). */ > > +CHECK_REG(OUTPORT); > > #define MFF_LOG_TUN_OFPORT MFF_REG5 /* 16..31 of the 32 bits */ > > +CHECK_REG(TUN_OFPORT); > > > > /* Logical registers. > > * > > * Make sure these don't overlap with the logical fields! */ > > #define MFF_LOG_REG0 MFF_REG0 > > +CHECK_REG(REG0); > > #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG4 > > +CHECK_REG(LB_ORIG_DIP_IPV4); > > #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 > > +CHECK_REG(LB_ORIG_TP_DPORT); > > > > #define MFF_LOG_XXREG0 MFF_XXREG0 > > Missed this one. > > > #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1 > > +CHECK_XXREG(LB_ORIG_DIP_IPV6); > > > > #define MFF_N_LOG_REGS 10 > > We may add another assertion here that: > MFF_LOG_REG0 + MFF_N_LOG_REGS <= MFF_REG0 + OVN_FLOW_N_REGS_SUPPORTED > For completeness. > > > > > #define MFF_LOG_LB_AFF_MATCH_IP4_ADDR MFF_REG4 > > +CHECK_REG(LB_AFF_MATCH_IP4_ADDR); > > #define MFF_LOG_LB_AFF_MATCH_IP6_ADDR MFF_XXREG1 > > +CHECK_XXREG(LB_AFF_MATCH_IP6_ADDR); > > #define MFF_LOG_LB_AFF_MATCH_PORT MFF_REG2 > > +CHECK_REG(LB_AFF_MATCH_PORT); > > > > #define MFF_LOG_RESULT_REG MFF_XXREG1 > > +CHECK_XXREG(RESULT_REG); > > > > #define MFF_LOG_CT_SAVED_STATE MFF_REG4 > > +CHECK_REG(CT_SAVED_STATE); > > > > #define MFF_LOG_REMOTE_OUTPORT MFF_REG1 /* Logical remote output > > * port (32 bits). */ > > +CHECK_REG(REMOTE_OUTPORT); > > > > /* Logical registers that are needed for backwards > > * compatibility with older northd versions. > > * XXX: All of them can be removed in 26.09. */ > > #define MFF_LOG_LB_ORIG_DIP_IPV4_OLD MFF_REG1 > > +CHECK_REG(LB_ORIG_DIP_IPV4_OLD); > > #define MFF_LOG_LB_AFF_MATCH_PORT_OLD MFF_REG8 > > +CHECK_REG(LB_AFF_MATCH_PORT_OLD); > > #define MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD MFF_XXREG0 > > +CHECK_XXREG(LB_AFF_MATCH_LS_IP6_ADDR_OLD); > > > > /* Maximum number of networks supported by 4-bit flags.network_id. */ > > #define OVN_MAX_NETWORK_ID \ > > diff --git a/lib/actions.c b/lib/actions.c > > index 6d1230e1e..2f1ff4992 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -1487,9 +1487,9 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > > } > > > > BUILD_ASSERT(MFF_LOG_CT_ZONE >= MFF_REG0); > > - BUILD_ASSERT(MFF_LOG_CT_ZONE < MFF_REG0 + FLOW_N_REGS); > > + BUILD_ASSERT(MFF_LOG_CT_ZONE < MFF_REG0 + OVN_FLOW_N_REGS_SUPPORTED); > > BUILD_ASSERT(MFF_LOG_DNAT_ZONE >= MFF_REG0); > > - BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + FLOW_N_REGS); > > + BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + OVN_FLOW_N_REGS_SUPPORTED); > > These are now duplicates of assertions in the logical-fields.h. > But we still need them here, because if the number of supported > ever changes, we'll need to take care of the use of the NXM_NX > prefix in the flows below. > > So, maybe we can replace these 4 assertions with one that checks > that OVN_FLOW_N_REGS_SUPPORTED == 16 ? WDYT? > > Maybe add a comment mentioning that this check makes sure that > all the used registers are within the NXM_NX code point prefix. > > May also add an assertion to encode_FWD_GROUP() checking that > MFF_LOG_OUTPORT - MFF_REG0 is within supported. For the same > reason. > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
