diff mbox

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

Message ID 1480690135-683-2-git-send-email-i.maximets@samsung.com
State Not Applicable
Delegated to: Daniele Di Proietto
Headers show

Commit Message

Ilya Maximets Dec. 2, 2016, 2:48 p.m. UTC 
This reduces code duplication. Additionally dp_netdev_free
will not reconfigure datapath on each removed port.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/dpif-netdev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Daniele Di Proietto Dec. 3, 2016, 2:21 a.m. UTC | #1 
On 02/12/2016 06:48, "Ilya Maximets" <i.maximets@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@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

>---
> 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
>
Daniele Di Proietto Dec. 3, 2016, 2:28 a.m. UTC | #2 
On 02/12/2016 18:21, "Daniele Di Proietto" <diproiettod@vmware.com> wrote:

>
>
>
>
>
>On 02/12/2016 06:48, "Ilya Maximets" <i.maximets@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@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
>>
diff mbox

Patch

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;
         }