Thanks Flavio for your inputs. My comments inline:
*Vasu Dasari* On Mon, Jun 24, 2019 at 3:58 PM Flavio Leitner <f...@sysclose.org> wrote: > On Wed, Jun 19, 2019 at 11:02:07PM -0400, Vasu Dasari wrote: > > Say an ARP entry is learnt on a OVS port and when such a port is deleted, > > learnt entry should be removed from the port. It would have be aged out > after > > ARP ageout time. This code will clean up immediately. > > > > Added test case(tunnel - neighbor entry add and deletion) in tunnel.at, > to > > verify neighbors are added and removed on deletion of a ports and > bridges. > > > > Discussion for this addition is at: > > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048754.html > > > > Signed-off-by: Vasu Dasari <vdas...@gmail.com> > > --- > > lib/tnl-neigh-cache.c | 20 +++++++++++++++++ > > lib/tnl-neigh-cache.h | 1 + > > ofproto/ofproto-dpif-xlate.c | 3 +++ > > tests/tunnel.at | 43 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 67 insertions(+) > > > > diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c > > index b28f9f1bb..8718405db 100644 > > --- a/lib/tnl-neigh-cache.c > > +++ b/lib/tnl-neigh-cache.c > > @@ -220,6 +220,26 @@ tnl_neigh_cache_run(void) > > } > > } > > > > +void > > +tnl_neigh_flush(const char br_name[]) > > It seems the file uses a convention declaring using IFNAMSIZ > Sure. I did not notice that. Will fix this. > > > +{ > > + struct tnl_neigh_entry *neigh; > > + bool changed = false; > > + > > + ovs_mutex_lock(&mutex); > > + CMAP_FOR_EACH(neigh, cmap_node, &table) { > > + if (nullable_string_is_equal(neigh->br_name, br_name)) { > > + tnl_neigh_delete(neigh); > > + changed = true; > > Do you expect to match on additional entries? Otherwise it > could bail out here. > > Say there are two or more neighbors on the port or on bridge, then by bailing out we would be missing others. So, will leave it there. > > > + } > > + } > > + ovs_mutex_unlock(&mutex); > > + > > + if (changed) { > > + seq_change(tnl_conf_seq); > > + } > > +} > > + > > static void > > tnl_neigh_cache_flush(struct unixctl_conn *conn, int argc OVS_UNUSED, > > const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) > > diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h > > index fee8e6a6f..ded9c2f86 100644 > > --- a/lib/tnl-neigh-cache.h > > +++ b/lib/tnl-neigh-cache.h > > @@ -37,5 +37,6 @@ int tnl_neigh_lookup(const char dev_name[], const > struct in6_addr *dst, > > struct eth_addr *mac); > > void tnl_neigh_cache_init(void); > > void tnl_neigh_cache_run(void); > > +void tnl_neigh_flush(const char dev_name[]); > > > > #endif > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 73966a4e8..e0cd8edab 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -1481,6 +1481,9 @@ xlate_ofport_remove(struct ofport_dpif *ofport) > > ovs_assert(new_xcfg); > > > > xport = xport_lookup(new_xcfg, ofport); > > + if(xport) { > ^--- space needed here. > > > + tnl_neigh_flush(netdev_get_name(xport->netdev)); > > + } > > xlate_xport_remove(new_xcfg, xport); > > > Shouldn't this be in xlate_xport_remove()? > > My first attempt was to put the hook in xlate_xport_remove(). But, this function is also getting called from another code path as well xlate_txn_commit() which is getting called as part of type_run() callback for ofproto_class. type_run()(from ofproto-dpif.c) xlate_txn_commit() xlate_xcfg_free() xlate_xbridge_remove() xlate_xport_remove() >From documentation this function can be called from periodically and hence flushing ARP entries. So, chose to do the flush when a ofport is removed. However, as I mentioned in the discussion, the tnl IP address could > be on a device that doesn't belong to OVS at all. For example: > > br-tun > | > +----- vxlan0 --- remote-ipaddr=192.168.1.10 > | > +----- vxlan1 --- remote-ipaddr=192.168.2.10 > > And then you have: > eth0 --- 192.168.1.1/24 > > eth1 --- 192.168.2.1/24 > > In this case, if we took either eth0 or eth1 down, the cache is not > immediately flushed. > > The tnl endpoint must have a valid route, so I suggest to hook the > tnl_neigh_cache_flush into route-table.c which receives a notification > when a route changes. If a route is deleted, we should flush the > device's tnl cache. Doing so, would cover both userspace and kernel > datapath for OVS and non-OVS devices. > > I see what you are saying. Let me play with code a bit and resubmit patch. Thanks -Vasu > What do you think? > > Thanks, > fbl > > > > diff --git a/tests/tunnel.at b/tests/tunnel.at > > index 035c54f67..6d7550724 100644 > > --- a/tests/tunnel.at > > +++ b/tests/tunnel.at > > @@ -920,3 +920,46 @@ dnl which is not correct > > > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > + > > +AT_SETUP([tunnel - neighbor entry add and deletion]) > > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=gre \ > > + options:remote_ip=1.1.1.1 options:local_ip=2.2.2.2 \ > > + options:key=5 ofport_request=1 \ > > + -- add-port br0 p2 -- set Interface p2 type=gre \ > > + options:local_ip=3.3.3.3 options:remote_ip=4.4.4.4 \ > > + ofport_request=2]) > > +AT_CHECK([ovs-vsctl add-br br1 -- set bridge br1 datapath_type=dummy], > [0]) > > + > > +dnl Populate tunnel neighbor cache table > > +AT_CHECK([ > > + ovs-appctl tnl/arp/set p1 10.0.0.1 00:00:10:00:00:01 > > + ovs-appctl tnl/arp/set p2 10.0.1.1 00:00:10:00:01:01 > > + ovs-appctl tnl/arp/set br0 10.0.2.1 00:00:10:00:02:01 > > + ovs-appctl tnl/arp/set br1 20.0.0.1 00:00:20:00:00:01 > > +], [0], [stdout]) > > + > > +AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl > > +10.0.0.1 00:00:10:00:00:01 p1 > > +10.0.1.1 00:00:10:00:01:01 p2 > > +10.0.2.1 00:00:10:00:02:01 br0 > > +20.0.0.1 00:00:20:00:00:01 br1 > > +]) > > + > > +dnl neighbor table after deleting port p1 > > +AT_CHECK([ovs-vsctl del-port br0 p1],[0], [stdout]) > > +AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | grep -w p1 | sort], > [0], [dnl > > +]) > > + > > +dnl neighbor table after deleting bridge br0 > > +AT_CHECK([ovs-vsctl del-br br0],[0], [stdout]) > > +AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl > > +20.0.0.1 00:00:20:00:00:01 br1 > > +]) > > + > > +dnl neighbor table after deleting bridge br1 > > +AT_CHECK([ovs-vsctl del-br br1],[0], [stdout]) > > +AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > -- > > 2.17.2 (Apple Git-113) > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev