| Message ID | 20170808182332.24497-2-joe@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
Seems good to me! Acked-by: Paul Blakey <paulb@mellanox.com> Thanks On 08/08/2017 21:23, Joe Stringer wrote: > Commit 32b77c316d9982("dpif: Save added ports in a port map.") > introduced tracking of all dpif ports by taking a reference on each > available netdev when the dpif is opened, but it failed to clear out and > release references to these netdevs when the dpif is closed. > > One of the problems introduced by this was that upon clean exit of > ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device > was not deleted. This which could cause problems in subsequent start up. > Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed > this particular problem by not adding such devices to the netdev_ports > map, but the referencing/unreferencing upon dpif_open()/dpif_close() is > still not balanced. > > Balance the referencing of netdevs by introducing netdev_ports_flush() > and clearing these during dpif_close(). > > Fixes: 32b77c316d9982("dpif: Save added ports in a port map.") > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > v2: Update commit message. > Rebase. > v1: Initial posting. > --- > lib/dpif.c | 1 + > lib/netdev.c | 15 +++++++++++++++ > lib/netdev.h | 1 + > 3 files changed, 17 insertions(+) > > diff --git a/lib/dpif.c b/lib/dpif.c > index e71f6a3d1475..53bdf39f6e20 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -437,6 +437,7 @@ dpif_close(struct dpif *dpif) > struct registered_dpif_class *rc; > > rc = shash_find_data(&dpif_classes, dpif->dpif_class->type); > + netdev_ports_flush(dpif->dpif_class); > dpif_uninit(dpif, true); > dp_class_unref(rc); > } > diff --git a/lib/netdev.c b/lib/netdev.c > index 3e8b211857d7..94f9e486d8b1 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2284,6 +2284,21 @@ netdev_ports_remove(odp_port_t port_no, const struct dpif_class *dpif_class) > return ret; > } > > +void > +netdev_ports_flush(const struct dpif_class *class) > +{ > + struct port_to_netdev_data *data, *next; > + > + ovs_mutex_lock(&netdev_hmap_mutex); > + HMAP_FOR_EACH_SAFE (data, next, node, &port_to_netdev) { > + if (data->dpif_class == class) { > + netdev_port_data_destroy(data); > + } > + } > + > + ovs_mutex_unlock(&netdev_hmap_mutex); > +} > + > odp_port_t > netdev_ifindex_to_odp_port(int ifindex) > { > diff --git a/lib/netdev.h b/lib/netdev.h > index f8482f787e39..2d02be5826f6 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -215,6 +215,7 @@ int netdev_ports_insert(struct netdev *, const struct dpif_class *, > struct dpif_port *); > struct netdev *netdev_ports_get(odp_port_t port, const struct dpif_class *); > int netdev_ports_remove(odp_port_t port, const struct dpif_class *); > +void netdev_ports_flush(const struct dpif_class *); > odp_port_t netdev_ifindex_to_odp_port(int ifindex); > struct netdev_flow_dump **netdev_ports_flow_dump_create( > const struct dpif_class *, >
On Tue, Aug 8, 2017 at 11:23 AM, Joe Stringer <joe@ovn.org> wrote: > Commit 32b77c316d9982("dpif: Save added ports in a port map.") > introduced tracking of all dpif ports by taking a reference on each > available netdev when the dpif is opened, but it failed to clear out and > release references to these netdevs when the dpif is closed. > > One of the problems introduced by this was that upon clean exit of > ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device > was not deleted. This which could cause problems in subsequent start up. > Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed > this particular problem by not adding such devices to the netdev_ports > map, but the referencing/unreferencing upon dpif_open()/dpif_close() is > still not balanced. > > Balance the referencing of netdevs by introducing netdev_ports_flush() > and clearing these during dpif_close(). > > Fixes: 32b77c316d9982("dpif: Save added ports in a port map.") > Signed-off-by: Joe Stringer <joe@ovn.org> I have a slightly different take on the bug fix. I am not super familiar with this code, so it could be wrong. Tracing the calling API, dpif_open -> do_open -> netdev_ports_insert(). So it would make sense for dpif_close to call netdev_ports_remove() to balance the reference issue mentioned in the comment. If true, then netdev_ports_flush() (previous patch) is not necessary. Currently, netdev_ports_remove() does not clean up ifindex_to_port map. Not clear it is correct.
On 8 August 2017 at 13:48, Andy Zhou <azhou@ovn.org> wrote: > On Tue, Aug 8, 2017 at 11:23 AM, Joe Stringer <joe@ovn.org> wrote: >> Commit 32b77c316d9982("dpif: Save added ports in a port map.") >> introduced tracking of all dpif ports by taking a reference on each >> available netdev when the dpif is opened, but it failed to clear out and >> release references to these netdevs when the dpif is closed. >> >> One of the problems introduced by this was that upon clean exit of >> ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device >> was not deleted. This which could cause problems in subsequent start up. >> Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed >> this particular problem by not adding such devices to the netdev_ports >> map, but the referencing/unreferencing upon dpif_open()/dpif_close() is >> still not balanced. >> >> Balance the referencing of netdevs by introducing netdev_ports_flush() >> and clearing these during dpif_close(). >> >> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.") >> Signed-off-by: Joe Stringer <joe@ovn.org> > > I have a slightly different take on the bug fix. I am not super > familiar with this code, > so it could be wrong. > > Tracing the calling API, dpif_open -> do_open -> netdev_ports_insert(). So it > would make sense for dpif_close to call netdev_ports_remove() to balance the > reference issue mentioned in the comment. If true, then > netdev_ports_flush() (previous > patch) is not necessary. I assume you mean netdev_port_data_destroy() ? I see, rather than taking this flush() approach I could look at adding DPIF_PORT_FOR_EACH() iteration code into the dpif_close(), which would more closely match the do_open() code. Then it'd be more obvious that these two paths are balanced. > Currently, netdev_ports_remove() does not clean up ifindex_to_port > map. Not clear > it is correct. You're right, and the ifidx is never freed either. I'll send a followup series to address these comments. Cheers, Joe
diff --git a/lib/dpif.c b/lib/dpif.c index e71f6a3d1475..53bdf39f6e20 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -437,6 +437,7 @@ dpif_close(struct dpif *dpif) struct registered_dpif_class *rc; rc = shash_find_data(&dpif_classes, dpif->dpif_class->type); + netdev_ports_flush(dpif->dpif_class); dpif_uninit(dpif, true); dp_class_unref(rc); } diff --git a/lib/netdev.c b/lib/netdev.c index 3e8b211857d7..94f9e486d8b1 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2284,6 +2284,21 @@ netdev_ports_remove(odp_port_t port_no, const struct dpif_class *dpif_class) return ret; } +void +netdev_ports_flush(const struct dpif_class *class) +{ + struct port_to_netdev_data *data, *next; + + ovs_mutex_lock(&netdev_hmap_mutex); + HMAP_FOR_EACH_SAFE (data, next, node, &port_to_netdev) { + if (data->dpif_class == class) { + netdev_port_data_destroy(data); + } + } + + ovs_mutex_unlock(&netdev_hmap_mutex); +} + odp_port_t netdev_ifindex_to_odp_port(int ifindex) { diff --git a/lib/netdev.h b/lib/netdev.h index f8482f787e39..2d02be5826f6 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -215,6 +215,7 @@ int netdev_ports_insert(struct netdev *, const struct dpif_class *, struct dpif_port *); struct netdev *netdev_ports_get(odp_port_t port, const struct dpif_class *); int netdev_ports_remove(odp_port_t port, const struct dpif_class *); +void netdev_ports_flush(const struct dpif_class *); odp_port_t netdev_ifindex_to_odp_port(int ifindex); struct netdev_flow_dump **netdev_ports_flow_dump_create( const struct dpif_class *,
Commit 32b77c316d9982("dpif: Save added ports in a port map.") introduced tracking of all dpif ports by taking a reference on each available netdev when the dpif is opened, but it failed to clear out and release references to these netdevs when the dpif is closed. One of the problems introduced by this was that upon clean exit of ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device was not deleted. This which could cause problems in subsequent start up. Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed this particular problem by not adding such devices to the netdev_ports map, but the referencing/unreferencing upon dpif_open()/dpif_close() is still not balanced. Balance the referencing of netdevs by introducing netdev_ports_flush() and clearing these during dpif_close(). Fixes: 32b77c316d9982("dpif: Save added ports in a port map.") Signed-off-by: Joe Stringer <joe@ovn.org> --- v2: Update commit message. Rebase. v1: Initial posting. --- lib/dpif.c | 1 + lib/netdev.c | 15 +++++++++++++++ lib/netdev.h | 1 + 3 files changed, 17 insertions(+)