[ovs-dev,PATCHv3,2/2] dpif: Clean up netdev_ports map on dpif_close().

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 clearing these during dpif_close().

Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
v3: Rather than flushing ports from this dpif, iterate ports and remove
    them.
v2: Update commit message.
    Rebase.
v1: Initial posting.
---
 lib/dpif.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On Tue, Aug 8, 2017 at 5:10 PM, 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 clearing these during dpif_close().
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Signed-off-by: Joe Stringer <joe@ovn.org>

A minor comment below.
Acked-by: Andy Zhou <azhou@ovn.org>
> ---
> v3: Rather than flushing ports from this dpif, iterate ports and remove
>     them.
> v2: Update commit message.
>     Rebase.
> v1: Initial posting.
> ---
>  lib/dpif.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index e71f6a3d1475..eccd8607f17e 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -435,8 +435,17 @@ dpif_close(struct dpif *dpif)
>  {
>      if (dpif) {
>          struct registered_dpif_class *rc;
> +        struct dpif_port_dump port_dump;
> +        struct dpif_port dpif_port;
>
>          rc = shash_find_data(&dpif_classes, dpif->dpif_class->type);
> +
> +        DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
> +            if (dpif_is_internal_port(dpif_port.type)) {
> +                continue;
> +            }
> +            netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
Would it be easier to read with:
                if (!dpif_is_internal_port(dpif_port.type)) {
                     netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
               }

> +        }
>          dpif_uninit(dpif, true);
>          dp_class_unref(rc);
>      }
> --
> 2.13.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 8 August 2017 at 18:33, Andy Zhou <azhou@ovn.org> wrote:
> On Tue, Aug 8, 2017 at 5:10 PM, 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 clearing these during dpif_close().
>>
>> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>
> A minor comment below.
> Acked-by: Andy Zhou <azhou@ovn.org>
>> ---
>> v3: Rather than flushing ports from this dpif, iterate ports and remove
>>     them.
>> v2: Update commit message.
>>     Rebase.
>> v1: Initial posting.
>> ---
>>  lib/dpif.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index e71f6a3d1475..eccd8607f17e 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -435,8 +435,17 @@ dpif_close(struct dpif *dpif)
>>  {
>>      if (dpif) {
>>          struct registered_dpif_class *rc;
>> +        struct dpif_port_dump port_dump;
>> +        struct dpif_port dpif_port;
>>
>>          rc = shash_find_data(&dpif_classes, dpif->dpif_class->type);
>> +
>> +        DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
>> +            if (dpif_is_internal_port(dpif_port.type)) {
>> +                continue;
>> +            }
>> +            netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
> Would it be easier to read with:
>                 if (!dpif_is_internal_port(dpif_port.type)) {
>                      netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
>                }

Yes, thanks for the suggestion. I applied this feedback and pushed the
patch to master.