On 7/23/25 8:40 AM, Han Zhou wrote: > On Thu, Jul 10, 2025 at 1:52 AM Dumitru Ceara <[email protected]> wrote: >> >> On 7/9/25 11:35 PM, Han Zhou wrote: >>> On Wed, Jul 9, 2025 at 11:38 AM Dumitru Ceara <[email protected]> wrote: >>>> >>>> On 7/9/25 8:01 PM, Han Zhou wrote: >>>>> On Wed, Dec 11, 2024 at 5:35 AM Dumitru Ceara <[email protected]> > wrote: >>>>>> >>>>>> On 12/10/24 12:35 PM, Ales Musil wrote: >>>>>>> On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <[email protected]> >>>>> wrote: >>>>>>> >>>>>>>> The actions that retrieve the original tuple destination IPs and >>> ports >>>>>>>> were used in the following way in ovn-northd: >>>>>>>> >>>>>>>> REG_ORIG_DIP_IPV4 " = ct_nw_dst() >>>>>>>> REG_ORIG_DIP_IPV6 " = ct_ip6_dst() >>>>>>>> REG_ORIG_TP_DPORT " = ct_tp_dst() >>>>>>>> >>>>>>>> Where: >>>>>>>> REG_ORIG_DIP_IPV4 is reg1 >>>>>>>> REG_ORIG_DIP_IPV6 is xxreg1 >>>>>>>> REG_ORIG_TP_DPORT is reg2[0..15] >>>>>>>> >>>>>>>> While the actions use a different intermediate register: >>>>>>>> ct_nw_dst() was reg4<-0; reg4<-ct_nw_dst >>>>>>>> ct_ip6_dst() was xxreg0<-0; xxreg0<-ct_ip6_dst >>>>>>>> ct_tp_dst() was reg8[0..15]<-0; reg8[0..15]<-ct_tp_dst >>>>>>>> >>>>>>>> We can reduce the set of registers we use in the OVN pipeline by >>>>>>>> changing the action definition to use the same registers ovn-northd >>>>> uses >>>>>>>> as final destination for the ct_*_dst() values. This will generate >>> the >>>>>>>> following openflow rules: >>>>>>>> >>>>>>>> REG_ORIG_DIP_IPV4 " = ct_nw_dst() >>>>>>>> reg1<-0; reg1<-ct_nw_dst; reg1<-reg1 >>>>>>>> REG_ORIG_DIP_IPV6 " = ct_ip6_dst() >>>>>>>> xxreg1<-0; xxreg1<-ct_ip6_dst; xxreg1<-xxreg1 >>>>>>>> REG_ORIG_TP_DPORT " = ct_tp_dst() >>>>>>>> reg2[0..15]<-0; reg2[0..15]<-ct_tp_dst; > reg2[0..15]<-reg2[0..15] >>>>>>>> >>>>>>>> As Ilya Maximets points out, overlapping source and destination are >>>>>>>> well defined for move actions: >>>>>>>> >>>>>>>> >>>>> >>> > https://opennetworking.org/wp-content/uploads/2014/10/openflow-switch-v1.5.0.noipr.pdf >>>>>>>> >>>>>>>> This action must behaves properly when src_oxm_id overlaps with >>>>>>>> dst_oxm_id, that is, it behaves as if src_oxm_id were copied out >>>>>>>> to a temporary buffer, then the temporary buffer copied to >>>>>>>> dst_oxm_id, if this is not possible the switch must reject the >>>>>>>> Copy Field action with a Bad Set Type error message. >>>>>>>> >>>>>>>> OpenvSwitch doesn't reject such actions. >>>>>>>> >>>>>>>> Fixes: 0f806cf08c36 ("Fix load balanced hairpin traffic for >>> fragmented >>>>>>>> packets.") >>>>>>>> Signed-off-by: Dumitru Ceara <[email protected]> >>>>>>>> --- >>>>>>>> include/ovn/logical-fields.h | 7 ++++--- >>>>>>>> tests/ovn.at | 8 ++++---- >>>>>>>> 2 files changed, 8 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/include/ovn/logical-fields.h >>>>> b/include/ovn/logical-fields.h >>>>>>>> index d563e044cb..70c6b93c41 100644 >>>>>>>> --- a/include/ovn/logical-fields.h >>>>>>>> +++ b/include/ovn/logical-fields.h >>>>>>>> @@ -60,9 +60,10 @@ enum ovn_controller_event { >>>>>>>> #define MFF_LOG_LB_AFF_MATCH_LR_IP6_ADDR MFF_XXREG1 >>>>>>>> #define MFF_LOG_LB_AFF_MATCH_PORT MFF_REG8 >>>>>>>> >>>>>>>> -#define MFF_LOG_CT_ORIG_NW_DST_ADDR MFF_REG4 >>>>>>>> -#define MFF_LOG_CT_ORIG_IP6_DST_ADDR MFF_XXREG0 >>>>>>>> -#define MFF_LOG_CT_ORIG_TP_DST_PORT MFF_REG8 >>>>>>>> +#define MFF_LOG_CT_ORIG_NW_DST_ADDR MFF_REG1 /* >>>>>>>> REG_ORIG_DIP_IPV4 */ >>>>>>>> +#define MFF_LOG_CT_ORIG_IP6_DST_ADDR MFF_XXREG1 /* >>>>>>>> REG_ORIG_DIP_IPV6 */ >>>>>>>> +#define MFF_LOG_CT_ORIG_TP_DST_PORT MFF_REG2 /* >>>>>>>> REG_ORIG_TP_DPORT >>>>>>>> + * (bits >>>>> 0..15). */ >>>>>>>> >>>>>>>> void ovn_init_symtab(struct shash *symtab); >>>>>>>> >>>>>>>> diff --git a/tests/ovn.at b/tests/ovn.at >>>>>>>> index 15b78f4c37..03fd2090fc 100644 >>>>>>>> --- a/tests/ovn.at >>>>>>>> +++ b/tests/ovn.at >>>>>>>> @@ -2420,10 +2420,10 @@ mac_cache_use; >>>>>>>> >>>>>>>> # ct_nw_dst() >>>>>>>> reg1 = ct_nw_dst(); >>>>>>>> - encodes as >>>>>>>> >>>>> >>> > set_field:0->reg4,resubmit(,OFTABLE_CT_ORIG_NW_DST_LOAD),move:NXM_NX_REG4[[]]->NXM_NX_XXREG0[[64..95]] >>>>>>>> + encodes as >>>>>>>> >>>>> >>> > set_field:0->reg1,resubmit(,OFTABLE_CT_ORIG_NW_DST_LOAD),move:NXM_NX_REG1[[]]->NXM_NX_XXREG0[[64..95]] >>>>>>>> >>>>>>>> xreg1[[3..34]] = ct_nw_dst(); >>>>>>>> - encodes as >>>>>>>> >>>>> >>> > set_field:0->reg4,resubmit(,OFTABLE_CT_ORIG_NW_DST_LOAD),move:NXM_NX_REG4[[]]->NXM_NX_XXREG0[[3..34]] >>>>>>>> + encodes as >>>>>>>> >>>>> >>> > set_field:0->reg1,resubmit(,OFTABLE_CT_ORIG_NW_DST_LOAD),move:NXM_NX_REG1[[]]->NXM_NX_XXREG0[[3..34]] >>>>>>>> >>>>>>>> reg1[[3..34]] = ct_nw_dst(); >>>>>>>> Cannot select bits 3 to 34 of 32-bit field reg1. >>>>>>>> @@ -2442,7 +2442,7 @@ ct_nw_dst(); >>>>>>>> >>>>>>>> # ct_ip6_dst() >>>>>>>> xxreg1 = ct_ip6_dst(); >>>>>>>> - encodes as >>>>>>>> >>>>> >>> > set_field:0/0xffffffffffffffff->xxreg0,set_field:0/0xffffffffffffffff0000000000000000->xxreg0,resubmit(,OFTABLE_CT_ORIG_IP6_DST_LOAD),move:NXM_NX_XXREG0[[]]->NXM_NX_XXREG1[[]] >>>>>>>> + encodes as >>>>>>>> >>>>> >>> > set_field:0/0xffffffffffffffff->xxreg1,set_field:0/0xffffffffffffffff0000000000000000->xxreg1,resubmit(,OFTABLE_CT_ORIG_IP6_DST_LOAD),move:NXM_NX_XXREG1[[]]->NXM_NX_XXREG1[[]] >>>>>>>> >>>>>>>> reg1 = ct_ip6_dst(); >>>>>>>> Cannot use 32-bit field reg1[[0..31]] where 128-bit field is >>>>> required. >>>>>>>> @@ -2455,7 +2455,7 @@ ct_ip6_dst(); >>>>>>>> >>>>>>>> # ct_tp_dst() >>>>>>>> reg1[[0..15]] = ct_tp_dst(); >>>>>>>> - encodes as >>>>>>>> >>>>> >>> > set_field:0/0xffff->reg8,resubmit(,OFTABLE_CT_ORIG_TP_DST_LOAD),move:NXM_NX_REG8[[0..15]]->NXM_NX_XXREG0[[64..79]] >>>>>>>> + encodes as >>>>>>>> >>>>> >>> > set_field:0/0xffff->reg2,resubmit(,OFTABLE_CT_ORIG_TP_DST_LOAD),move:NXM_NX_REG2[[0..15]]->NXM_NX_XXREG0[[64..79]] >>>>>>>> >>>>>>>> reg1 = ct_tp_dst(); >>>>>>>> Cannot use 32-bit field reg1[[0..31]] where 16-bit field is >>>>> required. >>>>>>>> -- >>>>>>>> 2.46.2 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> dev mailing list >>>>>>>> [email protected] >>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>>>> >>>>>>>> >>>>>>> Looks good to me, thanks. >>>>>>> >>>>>>> Acked-by: Ales Musil <[email protected]> >>>>>>> >>>>>> >>>>>> Thanks, Ales! Applied to main and backported to 24.09 and 24.03. >>>>>> >>>>>> Regards, >>>>>> Dumitru >>>>>> >>>>> >>>> >>>> Hi Han, >>>> >>>>> Hi Dumitru, Ales, I am trying to figure out the current register usage >>> and >>>>> saw this old commit. I am not sure I understand the motivation of this >>>>> patch. Is there a problem of using different registers for > intermediate >>>> >>>> There's no problem per se but we were using two registers instead of >>>> one. The intermediate register was overwritten and then its new value >>>> immediately copied to the final destination register. >>>> >>>> The goal of the patch was to free up registers for other things in we >>>> might need to do in the pipeline. >>>> >>>>> usage? Although this patch tried to use the same register for >>> intermediate >>>>> and final destination, a later commit from Ales 91988089c5 ("northd: >>>>> Consolidate register usage in logical flows.") already broke it again. >>>>> Shall we fix that again? >>>>> >>>> >>>> I guess we probably should. Maybe this time we should change the way >>>> the ct_*_dst() actions are encoded? I'm not sure how to prevent this >>>> from happening again in the future though. >>>> >>> >> >> Hi Han, >> >>> I think the right way to encode actions that require intermediate > registers >>> is to save the original content of the register on the stack beforehand > and >>> restore it afterwards. This way the intermediate registers won't really >>> occupy the register space, and we don't need to worry about conflict >>> usage/overwriting. Otherwise, it is very hard to maintain and not even > easy >>> to ensure the register is not overwritten accidentally if the same > action >>> is called at different stages. >>> I will go ahead with this approach if it makes sense. What do you think? >>> >> >> Sure, I was thinking of something similar initially too but I'm not sure >> how we can easily implement it. I might be missing something though. >> >> For example: >> >> reg4 = ct_nw_dst(); >> >> Is actually parsed as the following sequence of logical actions today: >> >> // ct_nw_dst() >> LOAD(reg1, 0) >> RESUBMIT(table=81) // This might write reg1. >> LOAD(reg4, reg1) >> >> So the problem I saw was that we're losing the information we had in reg1. >> >> To fix it, IIUC, you're suggesting to use the stack, I assume something >> like: >> >> // ct_nw_dst() >> PUSH(reg1) >> LOAD(reg1, 0) >> RESUBMIT(table=81) // This might write reg1. >> LOAD(reg4, reg1) >> POP(reg1) >> >> But the problem is that we're mixing the encoding of two logical actions >> (the push/pop are for the ct_nw_dst() call while the load(reg4, ..) is >> for the assignment). >> >> I guess, the correct way is to have a proper calling convention for all >> function-like actions like ct_nw_dst(), get_fdb(), etc. And ensure we >> always return the value in a predefined way, maybe always push it on the >> stack? > > Hi Dumitru, >
Hi Han, > I managed to encapsulate the push/pop in the actions themselves. > Please take a look at the patch-5 in the series: > https://patchwork.ozlabs.org/project/ovn/list/?series=466312 > Nice, I see the series is already merged, it's a very nice improvement, thanks a lot! Regards, Dumitru > Thanks, > Han >> >> Regards, >> Dumitru >> >>> Regards, >>> Han >>> >>>>> Thanks, >>>>> Han >>>>> >>>> >>>> Regards, >>>> Dumitru >>>> >>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> dev mailing list >>>>>> [email protected] >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> >>>> >>> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
