> Hi Lorenzo,

Hi Mark,

thx for the review.

> 
> I have a question about this, and it may be because I'm not 100% clear about
> the use case. Why are new flows only added for ARP responses? What about
> gARPs that use op=1? Do we not care about those?

Before applying this patch we update OFTABLE_MAC_CACHE_USE table just with IP
traffic. We need to refresh it even with ARP replies sent by the destination
when we try to refresh the stable entries.
I do not think we care about GARP here.

Regards,
Lorenzo

> 
> On 2/19/25 16:57, Lorenzo Bianconi wrote:
> > In order to make mac_binding ageing process more reliable, track ARP
> > replies/NA for the selected mac_binding raw in the OFTABLE_MAC_CACHE_USE
> > table.
> > 
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> >   controller/lflow.c     | 29 ++++++++++++++++++++++-------
> >   controller/mac-cache.c |  3 ++-
> >   tests/ovn.at           |  8 +++++++-
> >   3 files changed, 31 insertions(+), 9 deletions(-)
> > 
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index c2d280d5b..196539489 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -1356,6 +1356,12 @@ consider_neighbor_flow(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >       struct match get_arp_match = MATCH_CATCHALL_INITIALIZER;
> >       struct match lookup_arp_match = MATCH_CATCHALL_INITIALIZER;
> >       struct match mb_cache_use_match = MATCH_CATCHALL_INITIALIZER;
> > +    struct match lookup_arp_for_stats_match = MATCH_CATCHALL_INITIALIZER;
> > +
> > +    match_set_dl_src(&lookup_arp_match, mac_addr);
> > +    match_set_metadata(&lookup_arp_match, 
> > htonll(pb->datapath->tunnel_key));
> > +    match_set_reg(&lookup_arp_match, MFF_LOG_INPORT - MFF_REG0,
> > +                  pb->tunnel_key);
> >       if (strchr(ip, '.')) {
> >           ovs_be32 ip_addr;
> > @@ -1367,11 +1373,17 @@ consider_neighbor_flow(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >           match_set_reg(&get_arp_match, 0, ntohl(ip_addr));
> > -        match_set_reg(&lookup_arp_match, 0, ntohl(ip_addr));
> >           match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_ARP));
> > +        lookup_arp_for_stats_match = lookup_arp_match;
> > +
> > +        match_set_reg(&lookup_arp_match, 0, ntohl(ip_addr));
> >           match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IP));
> >           match_set_nw_src(&mb_cache_use_match, ip_addr);
> > +
> > +        match_set_arp_opcode_masked(&lookup_arp_for_stats_match, 2, 0xff);
> > +        match_set_arp_spa_masked(&lookup_arp_for_stats_match, ip_addr,
> > +                                 htonl(0xffffffff));
> >       } else {
> >           struct in6_addr ip6;
> >           if (!ipv6_parse(ip, &ip6)) {
> > @@ -1383,22 +1395,23 @@ consider_neighbor_flow(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >           memcpy(&value, &ip6, sizeof(value));
> >           match_set_xxreg(&get_arp_match, 0, ntoh128(value));
> > -        match_set_xxreg(&lookup_arp_match, 0, ntoh128(value));
> >           match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_IPV6));
> >           match_set_nw_proto(&lookup_arp_match, 58);
> >           match_set_icmp_code(&lookup_arp_match, 0);
> > +        lookup_arp_for_stats_match = lookup_arp_match;
> > +
> > +        match_set_xxreg(&lookup_arp_match, 0, ntoh128(value));
> >           match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IPV6));
> >           match_set_ipv6_src(&mb_cache_use_match, &ip6);
> > +
> > +        match_set_icmp_type(&lookup_arp_for_stats_match, 136);
> > +        match_set_nd_target(&lookup_arp_for_stats_match, &ip6);
> >       }
> >       match_set_metadata(&get_arp_match, htonll(pb->datapath->tunnel_key));
> >       match_set_reg(&get_arp_match, MFF_LOG_OUTPORT - MFF_REG0, 
> > pb->tunnel_key);
> > -    match_set_metadata(&lookup_arp_match, 
> > htonll(pb->datapath->tunnel_key));
> > -    match_set_reg(&lookup_arp_match, MFF_LOG_INPORT - MFF_REG0,
> > -                  pb->tunnel_key);
> > -
> >       match_set_dl_src(&mb_cache_use_match, mac_addr);
> >       match_set_reg(&mb_cache_use_match, MFF_LOG_INPORT - MFF_REG0,
> >                     pb->tunnel_key);
> > @@ -1418,7 +1431,6 @@ consider_neighbor_flow(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >       ofpbuf_clear(&ofpacts);
> >       put_load(&value, sizeof value, MFF_LOG_FLAGS, MLF_LOOKUP_MAC_BIT, 1,
> >                &ofpacts);
> > -    match_set_dl_src(&lookup_arp_match, mac_addr);
> >       ofctrl_add_flow(flow_table, OFTABLE_MAC_LOOKUP, priority,
> >                       b ? b->header_.uuid.parts[0] : 
> > smb->header_.uuid.parts[0],
> >                       &lookup_arp_match, &ofpacts,
> > @@ -1426,6 +1438,9 @@ consider_neighbor_flow(struct ovsdb_idl_index 
> > *sbrec_port_binding_by_name,
> >       if (b) {
> >           ofpbuf_clear(&ofpacts);
> > +        ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
> > +                        b->header_.uuid.parts[0], 
> > &lookup_arp_for_stats_match,
> > +                        &ofpacts, &b->header_.uuid);
> >           ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
> >                           b->header_.uuid.parts[0], &mb_cache_use_match,
> >                           &ofpacts, &b->header_.uuid);
> > diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> > index 580ca96b3..9d05c1950 100644
> > --- a/controller/mac-cache.c
> > +++ b/controller/mac-cache.c
> > @@ -361,7 +361,8 @@ mac_binding_stats_process_flow_stats(struct ovs_list 
> > *stats_list,
> >           .mac = ofp_stats->match.flow.dl_src
> >       };
> > -    if (ofp_stats->match.flow.dl_type == htons(ETH_TYPE_IP)) {
> > +    if (ofp_stats->match.flow.dl_type == htons(ETH_TYPE_IP) ||
> > +        ofp_stats->match.flow.dl_type == htons(ETH_TYPE_ARP)) {
> >           stats->data.mb.ip = 
> > in6_addr_mapped_ipv4(ofp_stats->match.flow.nw_src);
> >       } else {
> >           stats->data.mb.ip = ofp_stats->match.flow.ipv6_src;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 9b498aa0c..4bc6f1401 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -35522,7 +35522,11 @@ port_key_1=$(printf "0x%x" $(as hv1 fetch_column 
> > port_binding tunnel_key logical
> >   dp_key_2=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key 
> > external_ids:name=gw-2))
> >   port_key_2=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key 
> > logical_port=gw-2-public))
> > -table=" table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
> >  actions=drop
> > +table=" table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:10,arp_spa=192.168.10.10,arp_op=2
> >  actions=drop
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:20,arp_spa=192.168.10.20,arp_op=2
> >  actions=drop
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,arp_spa=192.168.10.10,arp_op=2
> >  actions=drop
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,arp_spa=192.168.10.20,arp_op=2
> >  actions=drop
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
> >  actions=drop
> >    table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_1},metadata=${dp_key_1},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
> >  actions=drop
> >    table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
> >  actions=drop
> >    table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
> >  actions=drop"
> > @@ -35570,6 +35574,8 @@ OVS_WAIT_UNTIL([
> >   ])
> >   AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int 
> > table=OFTABLE_MAC_CACHE_USE --no-stats | strip_cookie | sort], [0], [dnl
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,arp_spa=192.168.10.10,arp_op=2
> >  actions=drop
> > + table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,arp,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,arp_spa=192.168.10.20,arp_op=2
> >  actions=drop
> >    table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:10,nw_src=192.168.10.10
> >  actions=drop
> >    table=OFTABLE_MAC_CACHE_USE, 
> > priority=100,ip,reg14=${port_key_2},metadata=${dp_key_2},dl_src=00:00:00:00:10:20,nw_src=192.168.10.20
> >  actions=drop
> >   ])
> 
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to