Hi Mark, thanks for your comment, I replied inline
On 24/03/2016 10:17, "Kavanagh, Mark B" <mark.b.kavan...@intel.com> wrote: >Hi Daniele, > >One comment inline. > >Cheers, >Mark > >>-----Original Message----- >>From: Daniele Di Proietto [mailto:diproiet...@vmware.com] >>Sent: Wednesday, March 23, 2016 6:37 PM >>To: dev@openvswitch.org >>Cc: Ben Pfaff; Kavanagh, Mark B; Ilya Maximets; Daniele Di Proietto >>Subject: [PATCH v5 08/12] dpif-netdev: Change pmd thread configuration >>in dpif_netdev_run(). >> >>Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com> >>Tested-by: Ilya Maximets <i.maxim...@samsung.com> >>Acked-by: Ilya Maximets <i.maxim...@samsung.com> >>--- >> lib/dpif-netdev.c | 140 >>++++++++++++++++++++++++++++++---------------------- >> lib/dpif-provider.h | 3 +- >> 2 files changed, 83 insertions(+), 60 deletions(-) >> >>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>index 66c0b19..6aaeaeb 100644 >>--- a/lib/dpif-netdev.c >>+++ b/lib/dpif-netdev.c >>@@ -223,7 +223,9 @@ struct dp_netdev { >> ovsthread_key_t per_pmd_key; >> >> /* Cpu mask for pin of pmd threads. */ >>+ char *requested_pmd_cmask; >> char *pmd_cmask; >>+ >> uint64_t last_tnl_conf_seq; >> }; >> >>@@ -2447,82 +2449,44 @@ 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 for rx queues is 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; >> } >> } >> >>- if (dp->pmd_cmask != NULL && cmask != NULL) { >>- return strcmp(dp->pmd_cmask, cmask); >>- } else { >>- return (dp->pmd_cmask != NULL || cmask != NULL); >>+ return false; >>+} >>+ >>+static bool >>+cmask_equals(const char *a, const char *b) >>+{ >>+ if (a && b) { >>+ return !strcmp(a, b); >> } >>+ >>+ return a == NULL && b == NULL; >> } >> >>-/* Resets pmd threads if the configuration for 'rxq's or cpu mask >>changes. */ >>+/* Changes the number or the affinity of pmd threads. The changes are >>actually >>+ * applied in dpif_netdev_run(). */ >> static int >> dpif_netdev_pmd_set(struct dpif *dpif, const char *cmask) >> { >> struct dp_netdev *dp = get_dp_netdev(dpif); >> >>- if (pmd_config_changed(dp, cmask)) { >>- struct dp_netdev_port *port; >>- >>- dp_netdev_destroy_all_pmds(dp); >>- >>- 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(port->netdev) >>- && port->latest_requested_n_rxq != requested_n_rxq) { >>- int i, err; >>- >>- /* Closes the existing 'rxq's. */ >>- for (i = 0; i < netdev_n_rxq(port->netdev); i++) { >>- netdev_rxq_close(port->rxq[i]); >>- port->rxq[i] = NULL; >>- } >>- port->n_rxq = 0; >>- >>- /* Sets the new rx queue config. */ >>- err = netdev_set_multiq(port->netdev, >>- ovs_numa_get_n_cores() + 1, >>- requested_n_rxq); >>- if (err && (err != EOPNOTSUPP)) { >>- VLOG_ERR("Failed to set dpdk interface %s rx_queue >>to:" >>- " %u", netdev_get_name(port->netdev), >>- requested_n_rxq); >>- return err; >>- } >>- port->latest_requested_n_rxq = requested_n_rxq; >>- /* If the set_multiq() above succeeds, reopens the >>'rxq's. */ >>- port->n_rxq = netdev_n_rxq(port->netdev); >>- port->rxq = xrealloc(port->rxq, sizeof *port->rxq * >>port->n_rxq); >>- for (i = 0; i < port->n_rxq; i++) { >>- netdev_rxq_open(port->netdev, &port->rxq[i], i); >>- } >>- } >>- } >>- /* 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_equals(dp->requested_pmd_cmask, cmask)) { >>+ free(dp->requested_pmd_cmask); >>+ dp->requested_pmd_cmask = cmask ? xstrdup(cmask) : NULL; >> } >> >> return 0; >>@@ -2622,6 +2586,58 @@ dp_netdev_process_rxq_port(struct >>dp_netdev_pmd_thread *pmd, >> } >> } >> >>+static void >>+reconfigure_pmd_threads(struct dp_netdev *dp) >>+{ >>+ struct dp_netdev_port *port; >>+ >>+ dp_netdev_destroy_all_pmds(dp); >>+ >>+ 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(port->netdev) >>+ && port->latest_requested_n_rxq != requested_n_rxq) { >>+ int i, err; >>+ >>+ /* Closes the existing 'rxq's. */ >>+ for (i = 0; i < port->n_rxq; i++) { >>+ netdev_rxq_close(port->rxq[i]); >>+ port->rxq[i] = NULL; >>+ } >>+ port->n_rxq = 0; >>+ >>+ /* Sets the new rx queue config. */ >>+ err = netdev_set_multiq(port->netdev, >>ovs_numa_get_n_cores() + 1, >>+ requested_n_rxq); >>+ if (err && (err != EOPNOTSUPP)) { >>+ VLOG_ERR("Failed to set dpdk interface %s rx_queue to: >>%u", >>+ netdev_get_name(port->netdev), >>+ requested_n_rxq); >>+ return; >>+ } >>+ port->latest_requested_n_rxq = requested_n_rxq; >>+ /* If the set_multiq() above succeeds, reopens the 'rxq's. >>*/ >>+ port->n_rxq = netdev_n_rxq(port->netdev); >>+ port->rxq = xrealloc(port->rxq, sizeof *port->rxq * >>port->n_rxq); >>+ for (i = 0; i < port->n_rxq; i++) { >>+ netdev_rxq_open(port->netdev, &port->rxq[i], i); > >What happens if this fails? i.e. if port->rxq[i] is NULL? > >Presumably this could lead to a segfault later on when attempting to >reference the queue's 'queue_id' field? That's true, we have to check the return value, even though it is unlikely to fail. The problem is not introduced by this patch, but it is also on current master. I've introduced a fix in patch 10("dpif-netdev: Fix reconfigure_pmd_threads()."), which fixes other similar problems in reconfigure_pmd_threads(). Thanks for spotting this! > >>+ } >>+ } >>+ } >>+ /* Reconfigures the cpu mask. */ >>+ ovs_numa_set_cpu_mask(dp->requested_pmd_cmask); >>+ free(dp->pmd_cmask); >>+ dp->pmd_cmask = dp->requested_pmd_cmask >>+ ? xstrdup(dp->requested_pmd_cmask) >>+ : NULL; >>+ >>+ /* Restores the non-pmd. */ >>+ dp_netdev_set_nonpmd(dp); >>+ /* Restores all pmd threads. */ >>+ dp_netdev_reset_pmd_threads(dp); >>+} >>+ >> /* Return true if needs to revalidate datapath flows. */ >> static bool >> dpif_netdev_run(struct dpif *dpif) >>@@ -2642,8 +2658,14 @@ dpif_netdev_run(struct dpif *dpif) >> } >> } >> } >>- ovs_mutex_unlock(&dp->non_pmd_mutex); >>+ >> dp_netdev_pmd_unref(non_pmd); >>+ ovs_mutex_unlock(&dp->non_pmd_mutex); >>+ >>+ if (!cmask_equals(dp->pmd_cmask, dp->requested_pmd_cmask) >>+ || pmd_n_rxq_changed(dp)) { >>+ reconfigure_pmd_threads(dp); >>+ } >> >> tnl_neigh_cache_run(); >> tnl_port_map_run(); >>diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >>index fbd370f..25f4280 100644 >>--- a/lib/dpif-provider.h >>+++ b/lib/dpif-provider.h >>@@ -319,7 +319,8 @@ struct dpif_class { >> >> /* If 'dpif' creates its own I/O polling threads, refreshes poll >>threads >> * configuration. 'cmask' configures the cpu mask for setting the >>polling >>- * threads' cpu affinity. */ >>+ * threads' cpu affinity. The implementation might postpone >>applying the >>+ * changes until run() is called. */ >> int (*poll_threads_set)(struct dpif *dpif, const char *cmask); >> >> /* Translates OpenFlow queue ID 'queue_id' (in host byte order) >>into a >>-- >>2.1.4 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
