On Mon, Feb 24, 2025 at 3:57 PM Mark Michelson <[email protected]> wrote:
> On 2/24/25 06:11, Lorenzo Bianconi wrote: > >> Hi Lorenzo, > > > > Hi Mark, > > > > thx for thre review. > > > >> > >> I'm not sure I understand the need for this patch. Based on the code in > >> patch 3, it seems like the idea is to differentiate IPv4 and IPv6 based > on > >> whether the flow uses reg0. If the flow uses reg0, then IPv4, otherwise, > >> IPv6. But couldn't we do the same thing by keeping the registers the > same > >> and checking, say, reg1? If reg1 is used in the flow, then we know that > >> xxreg0 uses reg0-reg3, so therefore it's IPv6? And if reg1 is not used, > then > >> it's IPv4? Or does it not work that way? > > > > Yes, the issue is we need to understand in table OFTABLE_MAC_BINDING if > this > > is an IPv4 or an IPv6 flow. I think it is mostly the same, your solution > seems > > a little bit hacky to me. What do you think? > > My thought was that if we don't have to change registers, then that > keeps things consistent between versions of OVN. My main concern was > with backports. If someone backports a patch from a version where OVN > uses xxreg1 to a version where OVN uses xxreg0 in OFTABLE_MAC_BINDING, > there is a higher likelihood that the backporter will mess something up > and cause bad behavior in the old OVN version. If the registers don't > change, then there is no backport risk. > > Even with the slightly "hacky" solution, if it works (and is commented > well) then it's less risky than changing the established registers. My > main concern was whether my suggestion would actually work or not. If a > flow references xxreg0, then would checking the match for reg1 actually > succeed? > Hi Mark, if I may chime in why we need this. I'm afraid it won't work, the xxreg0 occupies reg0-reg3, so reg0 can have something in it when xxreg0 is used but might as well not have anything at all. So we could have false positives for IPv6 addresses with zeroes at the end. I don't think there is a way to know if the match on the register wasn't requested at all or if it's just 0. > > > > > Regards, > > Lorenzo > > > >> > >> On 2/19/25 16:57, Lorenzo Bianconi wrote: > >>> This is a preliminary patch to add actving probing the mac binding > >>> entries in table OFTABLE_MAC_BINDING. > >>> > >>> Signed-off-by: Lorenzo Bianconi <[email protected]> > >>> --- > >>> controller/lflow.c | 2 +- > >>> lib/actions.c | 4 ++-- > >>> tests/ovn.at | 8 ++++---- > >>> 3 files changed, 7 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/controller/lflow.c b/controller/lflow.c > >>> index 196539489..860869f55 100644 > >>> --- a/controller/lflow.c > >>> +++ b/controller/lflow.c > >>> @@ -1393,7 +1393,7 @@ consider_neighbor_flow(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > >>> } > >>> ovs_be128 value; > >>> memcpy(&value, &ip6, sizeof(value)); > >>> - match_set_xxreg(&get_arp_match, 0, ntoh128(value)); > >>> + match_set_xxreg(&get_arp_match, 1, ntoh128(value)); > >>> match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_IPV6)); > >>> match_set_nw_proto(&lookup_arp_match, 58); > >>> diff --git a/lib/actions.c b/lib/actions.c > >>> index 7ec481e00..be1916e66 100644 > >>> --- a/lib/actions.c > >>> +++ b/lib/actions.c > >>> @@ -2235,7 +2235,7 @@ encode_GET_ND(const struct ovnact_get_mac_bind > *get_mac, > >>> const struct ovnact_encode_params *ep, > >>> struct ofpbuf *ofpacts) > >>> { > >>> - encode_get_mac(get_mac, MFF_XXREG0, ep, ofpacts); > >>> + encode_get_mac(get_mac, MFF_XXREG1, ep, ofpacts); > >>> } > >>> static void > >>> @@ -2494,7 +2494,7 @@ encode_LOOKUP_ND_IP(const struct > ovnact_lookup_mac_bind_ip *lookup_mac, > >>> const struct ovnact_encode_params *ep, > >>> struct ofpbuf *ofpacts) > >>> { > >>> - encode_lookup_mac_bind_ip(lookup_mac, MFF_XXREG0, ep, ofpacts); > >>> + encode_lookup_mac_bind_ip(lookup_mac, MFF_XXREG1, ep, ofpacts); > >>> } > >>> static void > >>> diff --git a/tests/ovn.at b/tests/ovn.at > >>> index 4bc6f1401..1889f5202 100644 > >>> --- a/tests/ovn.at > >>> +++ b/tests/ovn.at > >>> @@ -1586,10 +1586,10 @@ nd_na_router { eth.src = 12:34:56:78:9a:bc; > nd.tll = 12:34:56:78:9a:bc; outport > >>> # get_nd > >>> get_nd(outport, ip6.dst); > >>> - encodes as > push:NXM_NX_XXREG0[[]],push:NXM_NX_IPV6_DST[[]],pop:NXM_NX_XXREG0[[]],set_field:00:00:00:00:00:00->eth_dst,resubmit(,OFTABLE_MAC_BINDING),pop:NXM_NX_XXREG0[[]] > >>> + encodes as > push:NXM_NX_XXREG1[[]],push:NXM_NX_IPV6_DST[[]],pop:NXM_NX_XXREG1[[]],set_field:00:00:00:00:00:00->eth_dst,resubmit(,66),pop:NXM_NX_XXREG1[[]] > >>> has prereqs eth.type == 0x86dd > >>> get_nd(inport, xxreg0); > >>> - encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],set_field:00:00:00:00:00:00->eth_dst,resubmit(,OFTABLE_MAC_BINDING),pop:NXM_NX_REG15[[]] > >>> + encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_XXREG1[[]],push:NXM_NX_XXREG0[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],pop:NXM_NX_XXREG1[[]],set_field:00:00:00:00:00:00->eth_dst,resubmit(,66),pop:NXM_NX_XXREG1[[]],pop:NXM_NX_REG15[[]] > >>> get_nd; > >>> Syntax error at `;' expecting `('. > >>> get_nd(); > >>> @@ -1676,10 +1676,10 @@ reg0[[0]] = lookup_nd(inport, ip6.src, > ip6.dst); > >>> # lookup_nd_ip > >>> reg2[[0]] = lookup_nd_ip(inport, ip6.dst); > >>> - encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_XXREG0[[]],push:NXM_NX_IPV6_DST[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],pop:NXM_NX_XXREG0[[]],push:NXM_OF_ETH_DST[[]],set_field:0/0x40->reg10,resubmit(,OFTABLE_MAC_BINDING),move:NXM_NX_REG10[[6]]->NXM_NX_XXREG0[[32]],pop:NXM_OF_ETH_DST[[]],pop:NXM_NX_XXREG0[[]],pop:NXM_NX_REG15[[]] > >>> + encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_XXREG1[[]],push:NXM_NX_IPV6_DST[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],pop:NXM_NX_XXREG1[[]],push:NXM_OF_ETH_DST[[]],set_field:0/0x40->reg10,resubmit(,66),move:NXM_NX_REG10[[6]]->NXM_NX_XXREG0[[32]],pop:NXM_OF_ETH_DST[[]],pop:NXM_NX_XXREG1[[]],pop:NXM_NX_REG15[[]] > >>> has prereqs eth.type == 0x86dd > >>> reg3[[0]] = lookup_nd_ip(inport, nd.target); > >>> - encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_XXREG0[[]],push:NXM_NX_ND_TARGET[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],pop:NXM_NX_XXREG0[[]],push:NXM_OF_ETH_DST[[]],set_field:0/0x40->reg10,resubmit(,OFTABLE_MAC_BINDING),move:NXM_NX_REG10[[6]]->NXM_NX_XXREG0[[0]],pop:NXM_OF_ETH_DST[[]],pop:NXM_NX_XXREG0[[]],pop:NXM_NX_REG15[[]] > >>> + encodes as > push:NXM_NX_REG15[[]],push:NXM_NX_XXREG1[[]],push:NXM_NX_ND_TARGET[[]],push:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]],pop:NXM_NX_XXREG1[[]],push:NXM_OF_ETH_DST[[]],set_field:0/0x40->reg10,resubmit(,66),move:NXM_NX_REG10[[6]]->NXM_NX_XXREG0[[0]],pop:NXM_OF_ETH_DST[[]],pop:NXM_NX_XXREG1[[]],pop:NXM_NX_REG15[[]] > >>> has prereqs (icmp6.type == 0x87 || icmp6.type == 0x88) && > eth.type == 0x86dd && ip.proto == 0x3a && (eth.type == 0x800 || eth.type == > 0x86dd) && icmp6.code == 0 && eth.type == 0x86dd && ip.proto == 0x3a && > (eth.type == 0x800 || eth.type == 0x86dd) && ip.ttl == 0xff && (eth.type == > 0x800 || eth.type == 0x86dd) > >>> lookup_nd_ip; > >> > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
