Re: [ovs-dev] [PATCH ovn 2/3] controller: Use xxreg1 for lookup_nd_ip() and get_nd() actions.

[Mark Michelson]

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?


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 <lorenzo.bianc...@redhat.com>
---
   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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev