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
