[ovs-dev] [PATCH 3/3] dpif-netdev: Destroy pmd threads only if pmd-cpu-mask changed.

[Daniele Di Proietto]

Thanks for the patch

I have a few comments inline

On 08/02/2016 07:30, "Ilya Maximets" <i.maximets at samsung.com> wrote:

>Since 5f2ccb1c0d3b ("dpif: Allow adding ukeys for same flow by
>different pmds.") there is the possibility to reassign queues among
>pmd threads without restarting them and deleting the megaflow cache.
>
>So, reconfiguration can be performed without destroying of PMD threads
>if pmd-cpu-mask not changed.
>
>Reassignment of all rx queues done to achieve fair distribution.
>
>Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>---
> lib/dpif-netdev.c | 62
>+++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 20 deletions(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index e62f5f6..8069163 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -2373,27 +2373,31 @@ dpif_netdev_operate(struct dpif *dpif, struct
>dpif_op **ops, size_t n_ops)
>     }
> }
> 
>-/* Returns true if the configuration for rx queues or cpu mask
>- * is changed. */
>+/* Returns true if the configuration of rx queues changed. */
> static bool
>-pmd_config_changed(const struct dp_netdev *dp, const char *cmask)
>+pmd_n_rxq_changed(const struct dp_netdev *dp)
> {
>     struct dp_netdev_port *port;
> 
>     CMAP_FOR_EACH (port, node, &dp->ports) {
>-        struct netdev *netdev = port->netdev;
>-        int requested_n_rxq = netdev_requested_n_rxq(netdev);
>-        if (netdev_is_pmd(netdev)
>+        int requested_n_rxq = netdev_requested_n_rxq(port->netdev);
>+        if (netdev_is_pmd(port->netdev)
>             && port->latest_requested_n_rxq != requested_n_rxq) {
>             return true;
>         }
>     }
>+    return false;
>+}
> 
>+/* Returns true if cpu mask changed. */
>+static bool
>+pmd_cpu_mask_changed(const struct dp_netdev *dp, const char *cmask)
>+{
>     if (dp->pmd_cmask != NULL && cmask != NULL) {
>         return strcmp(dp->pmd_cmask, cmask);
>-    } else {
>-        return (dp->pmd_cmask != NULL || cmask != NULL);
>     }
>+
>+    return (dp->pmd_cmask != NULL || cmask != NULL);
> }
> 
> /* Resets pmd threads if the configuration for 'rxq's or cpu mask
>changes. */
>@@ -2401,11 +2405,20 @@ static int
> dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask)
> {
>     struct dp_netdev *dp = get_dp_netdev(dpif);
>+    bool cmask_changed = pmd_cpu_mask_changed(dp, cmask);
>+    struct dp_netdev_port *port;
> 
>-    if (pmd_config_changed(dp, cmask)) {
>-        struct dp_netdev_port *port;
>+    if (cmask_changed || pmd_n_rxq_changed(dp)) {

We could just return here if nothing is changed. That will save us
one level of indentation.

>+        if (cmask_changed) {
>+            dp_netdev_destroy_all_pmds(dp);
>+        } else {
>+            struct dp_netdev_pmd_thread *pmd;
> 
>-        dp_netdev_destroy_all_pmds(dp);
>+            CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>+                dp_netdev_pmd_clear_poll_list(pmd);
>+                dp_netdev_reload_pmd__(pmd);

I guess this is still more than necessary.  Have you thought about
reloading just touching the pmd threads involved with the ports that
changed configuration?

>+            }
>+        }
> 
>         CMAP_FOR_EACH (port, node, &dp->ports) {
>             struct netdev *netdev = port->netdev;
>@@ -2439,15 +2452,24 @@ dpif_netdev_pmd_set(struct dpif *dpif, const char
>*cmask)
>                 }
>             }
>         }
>-        /* Reconfigures the cpu mask. */
>-        ovs_numa_set_cpu_mask(cmask);
>-        free(dp->pmd_cmask);
>-        dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
>-
>-        /* Restores the non-pmd. */
>-        dp_netdev_set_nonpmd(dp);
>-        /* Restores all pmd threads. */
>-        dp_netdev_reset_pmd_threads(dp);
>+
>+        if (cmask_changed) {
>+            /* Reconfigures the cpu mask. */
>+            ovs_numa_set_cpu_mask(cmask);
>+            free(dp->pmd_cmask);
>+            dp->pmd_cmask = cmask ? xstrdup(cmask) : NULL;
>+
>+            /* Restores the non-pmd. */
>+            dp_netdev_set_nonpmd(dp);
>+            /* Restores all pmd threads. */
>+            dp_netdev_reset_pmd_threads(dp);
>+        } else {
>+            CMAP_FOR_EACH (port, node, &dp->ports) {
>+                if (netdev_is_pmd(port->netdev)) {
>+                    dp_netdev_add_port_to_pmds(dp, port);
>+                }
>+            }
>+        }
>     }
> 
>     return 0;
>-- 
>2.5.0
>