Forwarding these to preserve message history ---------- Forwarded message --------- From: Ilya Maximets <[email protected]> Date: Mon, 9 Feb 2026 at 14:56 Subject: Re: [PATCH ovn] Update FLOW_N_REGS check. To: Mj ponsonby <[email protected]>, Dumitru Ceara < [email protected]> Cc: <[email protected]>
On 2/9/26 3:42 PM, Mj ponsonby wrote: >> As discussed on the IRC meeting last Friday with Frode, I was planning >> to post a submodule update patch early this week once we have all the >> remaining bits on OVS side cleaned up. With the last bits of the HW >> offload changes merged, we can proceed here. > > I didnt mean to step on any toes, I am just building OVN for the new ubuntu > release and needed to get it to work with OVS. No worries, I didn't have any actual work done on this yet, it was just a plan. :) > >> 1. handle_route_msg() prototype fix: >> Remove the table_id from struct route_msg_handle_data and use the >> new 'table' argument of the callback instead. >> 2. The actual submodule bump to the tip of branch-3.7. >> 3. The build fix for the registers. More on that below. > > There is also a handle_route_msg patch posted, I can post a v2 more in line > with the way you want that done if needed. > > I am happy to rework both of these patches (and possibly put them in a > series alongside the submodule bump), Please, do. I'll be happy to review. It should be a single patch though that includes all the changes and the submodule update, otherwise it will not compile. Best regards, Ilya Maximets. > but if you want to post an update > to this patch feel free to do so. > > Thanks, MJ > > On Mon, 9 Feb 2026 at 14:10, Dumitru Ceara <[email protected] <mailto: [email protected]>> wrote: > > On 2/9/26 2:58 PM, Ilya Maximets wrote: > > On 2/9/26 11:04 AM, Dumitru Ceara wrote: > >> On 2/9/26 10:58 AM, MJ Ponsonby wrote: > >>> In OVS commit 3900653 changes FLOW_N_REGS from 16 to 32, this means that > >>> you hit the version incompatibility error when compiling with newer OVS. > >>> This expands this check to cover the value of 32 that it now expects > >>> allowing newer OVS and OVN to be compatible. > >>> > >>> This commit expects OVS to be at version 3.7.0. > >>> > >>> Signed-off-by: MJ Ponsonby <[email protected] <mailto: [email protected]>> > >>> --- > >> > >> Hi MJ, > >> > >> Thanks for the patch! > >> > >>> controller/pinctrl.c | 26 +++++++++----------------- > >>> 1 file changed, 9 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c > >>> index 7baf6b488..a4e83f744 100644 > >>> --- a/controller/pinctrl.c > >>> +++ b/controller/pinctrl.c > >>> @@ -6368,23 +6368,15 @@ static void > >>> reload_metadata(struct ofpbuf *ofpacts, const struct match *md) > >>> { > >>> enum mf_field_id md_fields[] = { > >>> -#if FLOW_N_REGS == 16 > >>> - MFF_REG0, > >>> - MFF_REG1, > >>> - MFF_REG2, > >>> - MFF_REG3, > >>> - MFF_REG4, > >>> - MFF_REG5, > >>> - MFF_REG6, > >>> - MFF_REG7, > >>> - MFF_REG8, > >>> - MFF_REG9, > >>> - MFF_REG10, > >>> - MFF_REG11, > >>> - MFF_REG12, > >>> - MFF_REG13, > >>> - MFF_REG14, > >>> - MFF_REG15, > >>> +#if FLOW_N_REGS == 32 > >>> + MFF_REG0, MFF_REG1, MFF_REG2, MFF_REG3, \ > >>> + MFF_REG4, MFF_REG5, MFF_REG6, MFF_REG7, \ > >>> + MFF_REG8, MFF_REG9, MFF_REG10, MFF_REG11, \ > >>> + MFF_REG12, MFF_REG13, MFF_REG14, MFF_REG15, \ > >>> + MFF_REG16, MFF_REG17, MFF_REG18, MFF_REG19, \ > >>> + MFF_REG20, MFF_REG21, MFF_REG22, MFF_REG23, \ > >>> + MFF_REG24, MFF_REG25, MFF_REG26, MFF_REG27, \ > >>> + MFF_REG28, MFF_REG29, MFF_REG30, MFF_REG31, \ > >> > >> I'm not sure this is the right way to go though. > >> > >> First of all we should bump the submodule version, otherwise all our CI > >> will break. > > Yes, though submodule bump will also require a few more changes to the > > code just to compile and maybe a few more to make it fully correct. > > > > As discussed on the IRC meeting last Friday with Frode, I was planning > > to post a submodule update patch early this week once we have all the > > remaining bits on OVS side cleaned up. With the last bits of the HW > > offload changes merged, we can proceed here. > > > > I can send a patch myself, or, MJ, if you want to do the work, I can > > leave it to you. Just let me know. > > > > The changes that are necessary: > > > > 1. handle_route_msg() prototype fix: > > Remove the table_id from struct route_msg_handle_data and use the > > new 'table' argument of the callback instead. > > 2. The actual submodule bump to the tip of branch-3.7. > > 3. The build fix for the registers. More on that below. > > > >> > >> This is a compile time check but there's no guarantee that the version > >> of OVS that's used at runtime actually supports 32 registers. > >> > >> E.g., if: > >> - OVS used at compilation time is 3.7 > >> - OVS used at runtime is 3.6 > >> > >> with your patch OVN will generate code that tries to clear the new > >> registers but the OVS used at runtime doesn't support those. > >> > >> We should instead use the capabilities exposed by OVS to determine the > >> actual number of runtime registers and only use them in OVN if the > >> underlying OVS supports them. > > I don't think it's necessary to do a runtime check at this point in time, > > it can be introduced together with the first actual usage of the new > > registers. For now we just need to make it compile and make sure that > > the code is clear in the way that it never uses more than 16 registers. > > > > The reload_metadata() code is used to re-set the values for registers > > that were previously received from the packet-in message, so it should > > not be a bit problem to just set all that were originally present. > > If it was the old OVS that sent a packet-in, then the message will not > > contain higher registers, it it was the new OVS, then it may. So, the > > only scenario where this is a problem is if we have new OVS send a > > packet-in, then OVS gets downgraded and ovn-controller sends a packet-out > > to the old OVS. But at the same it, it's probably fine to just drop this > > message entirely anyway, which will happen indirectly on the packet-out > > failure. > > > > At the same time, the OVN pipeline at his point in time should not > > contain new registers, so they should not be present in the packet-in > > message anyways. > > > > On the other hand, I agree that it may be better to just avoid dealing > > with these registers at all as they should not be present in the pipeline. > > What we can do is define a new variable: > > > > /* 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); > > > > Then we can use OVN_FLOW_N_REGS_SUPPORTED in the code instead of the > > FLOW_N_REGS. This way we will only restore 16 registers, but that should > > be all that we actually need. This will also cover the build assertions > > in the encode_ct_lb() that protect the use of NXM_NX_ registers. > > > > We can also add additional checks for all the existing definitions, > > if needed, by doing something like: > > > > #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); > > > > CHECK_REG(FLAGS); > > CHECK_REG(DNAT_ZONE); > > ... > > CHECK_XXREG(LB_AFF_MATCH_LS_IP6_ADDR_OLD); > > > > WDYT? > > > > Sounds like a good way forward to me. > > Thanks, > Dumitru > > > Best regards, Ilya Maximets. > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
