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]> >>> --- >> >> 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
