| Message ID | 20241111104327.65539-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] logical-fields: Reuse registers for ct_*_dst() actions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Recheck-request: github-robot On 11/11/24 11:43 AM, Dumitru Ceara 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 <dceara@redhat.com> > ---
On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 12/10/24 12:35 PM, Ales Musil wrote: > On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks, Ales! Applied to main and backported to 24.09 and 24.03. Regards, Dumitru
On Wed, Dec 11, 2024 at 5:35 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 12/10/24 12:35 PM, Ales Musil wrote: > > On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> > >> --- > >> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > Looks good to me, thanks. > > > > Acked-by: Ales Musil <amusil@redhat.com> > > > > Thanks, Ales! Applied to main and backported to 24.09 and 24.03. > > Regards, > Dumitru > 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 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? Thanks, Han > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/9/25 8:01 PM, Han Zhou wrote: > On Wed, Dec 11, 2024 at 5:35 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 12/10/24 12:35 PM, Ales Musil wrote: >>> On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> > 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 <dceara@redhat.com> >>>> --- >>>> 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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >>> Looks good to me, thanks. >>> >>> Acked-by: Ales Musil <amusil@redhat.com> >>> >> >> 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. > Thanks, > Han > Regards, Dumitru > >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jul 9, 2025 at 11:38 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/9/25 8:01 PM, Han Zhou wrote: > > On Wed, Dec 11, 2024 at 5:35 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 12/10/24 12:35 PM, Ales Musil wrote: > >>> On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> > > 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 <dceara@redhat.com> > >>>> --- > >>>> 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 > >>>> dev@openvswitch.org > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>> > >>>> > >>> Looks good to me, thanks. > >>> > >>> Acked-by: Ales Musil <amusil@redhat.com> > >>> > >> > >> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 7/9/25 11:35 PM, Han Zhou wrote: > On Wed, Jul 9, 2025 at 11:38 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 7/9/25 8:01 PM, Han Zhou wrote: >>> On Wed, Dec 11, 2024 at 5:35 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> On 12/10/24 12:35 PM, Ales Musil wrote: >>>>> On Mon, Nov 11, 2024 at 11:43 AM Dumitru Ceara <dceara@redhat.com> >>> 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 <dceara@redhat.com> >>>>>> --- >>>>>> 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 >>>>>> dev@openvswitch.org >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> >>>>>> >>>>> Looks good to me, thanks. >>>>> >>>>> Acked-by: Ales Musil <amusil@redhat.com> >>>>> >>>> >>>> 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? Regards, Dumitru > Regards, > Han > >>> Thanks, >>> Han >>> >> >> Regards, >> Dumitru >> >>> >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >
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.
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 <dceara@redhat.com> --- include/ovn/logical-fields.h | 7 ++++--- tests/ovn.at | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-)