[ovs-dev] [PATCH 1/3] dpif-netdev: Use do_del_port in reconfigure_datapath.

[Daniele Di Proietto]

On 02/12/2016 18:21, "Daniele Di Proietto" <diproiettod at vmware.com> wrote:

>
>
>
>
>
>On 02/12/2016 06:48, "Ilya Maximets" <i.maximets at samsung.com> wrote:
>
>>This reduces code duplication. Additionally dp_netdev_free
>>will not reconfigure datapath on each removed port.
>>
>>Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>
>I like reducing code duplication, but I'm not sure I like that
>do_del_port() calls reconfigure_datapath() and reconfigure_datapath()
>calls do_del_port().  Do you think we could avoid this "recursion"?
>
>Thanks,
>
>Daniele

Also, I forgot to mention that dp_netdev_free() is only called by
dpctl/del-dp, so I don't think is superimportant to optimize it

>
>>---
>> lib/dpif-netdev.c | 26 +++++++++++++-------------
>> 1 file changed, 13 insertions(+), 13 deletions(-)
>>
>>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>index f6552da..8e9a623 100644
>>--- a/lib/dpif-netdev.c
>>+++ b/lib/dpif-netdev.c
>>@@ -561,7 +561,8 @@ static void dp_netdev_free(struct dp_netdev *)
>> static int do_add_port(struct dp_netdev *dp, const char *devname,
>>                        const char *type, odp_port_t port_no)
>>     OVS_REQUIRES(dp->port_mutex);
>>-static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *)
>>+static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *,
>>+                        bool port_in_datapath)
>>     OVS_REQUIRES(dp->port_mutex);
>> static int dpif_netdev_open(const struct dpif_class *, const char *name,
>>                             bool create, struct dpif **);
>>@@ -1150,12 +1151,13 @@ dp_netdev_free(struct dp_netdev *dp)
>> 
>>     shash_find_and_delete(&dp_netdevs, dp->name);
>> 
>>+    dp_netdev_destroy_all_pmds(dp, true);
>>+
>>     ovs_mutex_lock(&dp->port_mutex);
>>     HMAP_FOR_EACH_SAFE (port, next, node, &dp->ports) {
>>-        do_del_port(dp, port);
>>+        do_del_port(dp, port, false);
>>     }
>>     ovs_mutex_unlock(&dp->port_mutex);
>>-    dp_netdev_destroy_all_pmds(dp, true);
>>     cmap_destroy(&dp->poll_threads);
>> 
>>     ovs_mutex_destroy(&dp->non_pmd_mutex);
>>@@ -1399,7 +1401,7 @@ dpif_netdev_port_del(struct dpif *dpif, odp_port_t port_no)
>> 
>>         error = get_port_by_number(dp, port_no, &port);
>>         if (!error) {
>>-            do_del_port(dp, port);
>>+            do_del_port(dp, port, true);
>>         }
>>     }
>>     ovs_mutex_unlock(&dp->port_mutex);
>>@@ -1495,13 +1497,16 @@ has_pmd_port(struct dp_netdev *dp)
>> }
>> 
>> static void
>>-do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
>>+do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port,
>>+            bool port_in_datapath)
>>     OVS_REQUIRES(dp->port_mutex)
>> {
>>     hmap_remove(&dp->ports, &port->node);
>>     seq_change(dp->port_seq);
>> 
>>-    reconfigure_datapath(dp);
>>+    if (port_in_datapath) {
>>+        reconfigure_datapath(dp);
>>+    }
>> 
>>     port_destroy(port);
>> }
>>@@ -3243,17 +3248,12 @@ reconfigure_datapath(struct dp_netdev *dp)
>>      * not being used by any pmd thread at the moment.  If a port fails to
>>      * reconfigure we remove it from the datapath. */
>>     HMAP_FOR_EACH(port, node, &dp->ports) {
>>-        int err;
>>-
>>         if (!port->need_reconfigure) {
>>             continue;
>>         }
>> 
>>-        err = port_reconfigure(port);
>>-        if (err) {
>>-            hmap_remove(&dp->ports, &port->node);
>>-            seq_change(dp->port_seq);
>>-            port_destroy(port);
>>+        if (port_reconfigure(port)) {
>>+            do_del_port(dp, port, false);
>>         } else {
>>             port->dynamic_txqs = netdev_n_txq(port->netdev) < wanted_txqs;
>>         }
>>-- 
>>2.7.4
>>