>
>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.
Yup - I noticed it while reviewing v5 of your patch, and thought to mention it,
seeing as you're working in this general area :)
>
>I've introduced a fix in patch 10("dpif-netdev: Fix
>reconfigure_pmd_threads()."),
>which fixes other similar problems in reconfigure_pmd_threads().
>
Awesome - thanks!
>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
