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. >
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? 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
