On Mon, Apr 3, 2023 at 5:55 AM Dumitru Ceara <[email protected]> wrote: > > On 4/3/23 11:47, Abhiram Sangana wrote: > > > > > >> On 29 Mar 2023, at 16:21, Dumitru Ceara <[email protected]> wrote: > >> > >> On 3/29/23 12:00, Abhiram Sangana wrote: > >>>>>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, > >>>>>> pb->header_.uuid.parts[0], &match, ofpacts_p, > >>>>>> &pb->header_.uuid); > >>>>>> > >>>>>> + if (zone_ids->drop) { > >>>>>> + /* Table 39, Priority 1. > >>>>>> + * ======================= > >>>>>> + * > >>>>>> + * Clear the logical registers (for consistent behavior with > >>>>>> packets > >>>>>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ > >>>>>> + match_init_catchall(&match); > >>>>>> + ofpbuf_clear(ofpacts_p); > >>>>>> + match_set_metadata(&match, htonll(dp_key)); > >>>>>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { > >>>>>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { > >>>>>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); > >>>>>> + } > >>>>>> + } > >>>> Why do we need this? Don't we load the CT zone anyway for "to-lport" > >>>> ACLs? Can't we also load the drop zone in the same place? > >>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone > >>> for “to-lport” ACLs (reg 13) but this happens before table=39 where > >>> registers 0 to 9 are cleared. > >> > >> Oh, I see now, it's because the drop zone is stored in reg9. I wanted > >> to suggest not allowing reg9 to be used as logical register anymore but > >> that's not really easy because it's used in the router pipeline: > >> > >> /* Register to store the result of check_pkt_larger action. */ > >> #define REGBIT_PKT_LARGER "reg9[1]" > >> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]" > >> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]" > >> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]" > >> #define REGBIT_KNOWN_ECMP_NH "reg9[5]" > >> #define REGBIT_KNOWN_LB_SESSION "reg9[6]" > >> > >> [...] > >> #define REG_ORIG_TP_DPORT_ROUTER "reg9[16..31]" > >> > >> Can we reuse one of the router-specific zones registers instead? > >> > >> #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway > >> router > >> * (32 bits). */ > >> #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway > >> router > >> * (32 bits). */ > > > > Currently, we seem to be allocating SNAT and DNAT zones for > > logical switches too and loading registers 11 and 12. > > > > You're right, we are. And we're also using the SNAT zone for LB > hairpin. But AFAICT we don't use the DNAT zone in the switch pipeline > (and I don't think we'll ever use it). > > > bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch. > > > > $ ovn-appctl ct-zone-list > > bb83b543-0d9d-409b-832c-4fc235355289_dnat 1 > > sw0p1 3 > > bb83b543-0d9d-409b-832c-4fc235355289_snat 2 > > bb83b543-0d9d-409b-832c-4fc235355289_drop 4 > > > > cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, > > idle_age=27, priority=100,in_port=1 > > actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8) > > > > cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, > > idle_age=27, priority=100,reg15=0x1,metadata=0x1 > > actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39) > > > > Is it ok to reuse these registers? > > > > I think it's OK to reuse the DNAT register; Numan do you agree?
In the switch pipeline we do NAT on the port zones and for the load balancer in the snat zone. So I think it should be fine. Thanks Numan > > Thanks! > > _______________________________________________ > 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
