On 8 August 2017 at 13:48, Andy Zhou <az...@ovn.org> wrote:
> On Tue, Aug 8, 2017 at 11:23 AM, Joe Stringer <j...@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 <j...@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.

dev mailing list

Reply via email to