On 2/9/26 3:10 PM, Dumitru Ceara 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]> >>>> --- >>> >>> 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.
OK. MJ, do you want to work on these changes or should I? Feel free to reuse any of the code snippets above (I didn't actually test any of them). The timeline here is that we should preferably merge the submodule bump this week, i.e. before branching. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
