[ovs-dev] [PATCH] tnl-neigh-cache: Purge learnt neighbors when port/bridge is deleted
Vasu Dasari
vdasari at gmail.com
Mon Jun 24 21:43:26 UTC 2019
Thanks Flavio for your inputs.
My comments inline:
*Vasu Dasari*
On Mon, Jun 24, 2019 at 3:58 PM Flavio Leitner <fbl at 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 <vdasari at 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)
> >
>
More information about the dev
mailing list