| Message ID | 1487688568-14820-3-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Darrell Ball |
| Headers | show |
2017-02-21 6:49 GMT-08:00 Ilya Maximets <i.maximets@samsung.com>: > Currently, change of 'pmd-cpu-mask' is very heavy operation. > It requires destroying of all the PMD threads and creating > them back. After that, all the threads will sleep until > ports' redistribution finished. > > This patch adds ability to not stop the datapath while > adjusting number/placement of PMD threads. All not affected > threads will forward traffic without any additional latencies. > > id-pool created for static tx queue ids to keep them sequential > in a flexible way. non-PMD thread will always have > static_tx_qid = 0 as it was before. > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Thanks for the patch The idea looks good to me. I'm still looking at the code, and I have one comment below > --- > lib/dpif-netdev.c | 119 +++++++++++++++++++++++++++++++++++++++++------------- > tests/pmd.at | 2 +- > 2 files changed, 91 insertions(+), 30 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 30907b7..6e575ab 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -48,6 +48,7 @@ > #include "fat-rwlock.h" > #include "flow.h" > #include "hmapx.h" > +#include "id-pool.h" > #include "latch.h" > #include "netdev.h" > #include "netdev-vport.h" > @@ -241,6 +242,9 @@ struct dp_netdev { > > /* Stores all 'struct dp_netdev_pmd_thread's. */ > struct cmap poll_threads; > + /* id pool for per thread static_tx_qid. */ > + struct id_pool *tx_qid_pool; > + struct ovs_mutex tx_qid_pool_mutex; > > /* Protects the access of the 'struct dp_netdev_pmd_thread' > * instance for non-pmd thread. */ > @@ -514,7 +518,7 @@ struct dp_netdev_pmd_thread { > /* Queue id used by this pmd thread to send packets on all netdevs if > * XPS disabled for this netdev. All static_tx_qid's are unique and less > * than 'cmap_count(dp->poll_threads)'. */ > - const int static_tx_qid; > + uint32_t static_tx_qid; > > struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and 'tx_ports'. */ > /* List of rx queues to poll. */ > @@ -594,6 +598,8 @@ static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp, > unsigned core_id); > static struct dp_netdev_pmd_thread * > dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position *pos); > +static void dp_netdev_del_pmd(struct dp_netdev *dp, > + struct dp_netdev_pmd_thread *pmd); > static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp, bool non_pmd); > static void dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread *pmd); > static void dp_netdev_add_port_tx_to_pmd(struct dp_netdev_pmd_thread *pmd, > @@ -1077,10 +1083,17 @@ create_dp_netdev(const char *name, const struct dpif_class *class, > atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); > > cmap_init(&dp->poll_threads); > + > + ovs_mutex_init(&dp->tx_qid_pool_mutex); > + /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD threads. */ > + dp->tx_qid_pool = id_pool_create(0, ovs_numa_get_n_cores() + 1); > + > ovs_mutex_init_recursive(&dp->non_pmd_mutex); > ovsthread_key_create(&dp->per_pmd_key, NULL); > > ovs_mutex_lock(&dp->port_mutex); > + /* non-PMD will be created before all other threads and will > + * allocate static_tx_qid = 0. */ > dp_netdev_set_nonpmd(dp); > > error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class, > @@ -1164,6 +1177,9 @@ dp_netdev_free(struct dp_netdev *dp) > dp_netdev_destroy_all_pmds(dp, true); > cmap_destroy(&dp->poll_threads); > > + ovs_mutex_destroy(&dp->tx_qid_pool_mutex); > + id_pool_destroy(dp->tx_qid_pool); > + > ovs_mutex_destroy(&dp->non_pmd_mutex); > ovsthread_key_delete(dp->per_pmd_key); > > @@ -3175,7 +3191,10 @@ reconfigure_pmd_threads(struct dp_netdev *dp) > { > struct dp_netdev_pmd_thread *pmd; > struct ovs_numa_dump *pmd_cores; > - bool changed = false; > + struct ovs_numa_info_core *core; > + struct hmapx to_delete = HMAPX_INITIALIZER(&to_delete); > + struct hmapx_node *node; > + int created = 0, deleted = 0; > > /* The pmd threads should be started only if there's a pmd port in the > * datapath. If the user didn't provide any "pmd-cpu-mask", we start > @@ -3188,45 +3207,62 @@ reconfigure_pmd_threads(struct dp_netdev *dp) > pmd_cores = ovs_numa_dump_n_cores_per_numa(NR_PMD_THREADS); > } > > - /* Check for changed configuration */ > - if (ovs_numa_dump_count(pmd_cores) != cmap_count(&dp->poll_threads) - 1) { > - changed = true; > - } else { > - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { > - if (pmd->core_id != NON_PMD_CORE_ID > - && !ovs_numa_dump_contains_core(pmd_cores, > - pmd->numa_id, > - pmd->core_id)) { > - changed = true; > - break; > - } > + /* Check for unwanted pmd threads */ > + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { > + if (pmd->core_id != NON_PMD_CORE_ID > + && !ovs_numa_dump_contains_core(pmd_cores, > + pmd->numa_id, > + pmd->core_id)) { > + hmapx_add(&to_delete, pmd); > } > } > + HMAPX_FOR_EACH (node, &to_delete) { > + pmd = (struct dp_netdev_pmd_thread *) node->data; > + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d destroyed.", > + pmd->numa_id, pmd->core_id); > + dp_netdev_del_pmd(dp, pmd); > + } > + deleted = hmapx_count(&to_delete); > + hmapx_destroy(&to_delete); > > - /* Destroy the old and recreate the new pmd threads. We don't perform an > - * incremental update because we would have to adjust 'static_tx_qid'. */ > - if (changed) { > - struct ovs_numa_info_core *core; > - struct ovs_numa_info_numa *numa; > - > - /* Do not destroy the non pmd thread. */ > - dp_netdev_destroy_all_pmds(dp, false); > - FOR_EACH_CORE_ON_DUMP (core, pmd_cores) { > - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd); > - > + /* Check for required new pmd threads */ > + FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { > + pmd = dp_netdev_get_pmd(dp, core->core_id); > + if (!pmd) { > + pmd = xzalloc(sizeof *pmd); > dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id); > - > pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd); > + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d created.", > + pmd->numa_id, pmd->core_id); > + created++; > + } else { > + dp_netdev_pmd_unref(pmd); > } > + } > + > + if (deleted || created) { > + struct ovs_numa_info_numa *numa; > > /* Log the number of pmd threads per numa node. */ > FOR_EACH_NUMA_ON_DUMP (numa, pmd_cores) { > - VLOG_INFO("Created %"PRIuSIZE" pmd threads on numa node %d", > + VLOG_INFO("There are %"PRIuSIZE" pmd threads on numa node %d", > numa->n_cores, numa->numa_id); > } > } > > ovs_numa_dump_destroy(pmd_cores); > + > + if (deleted > created) { > + /* Static tx qids are not sequential now. Mark all threads for > + * reload to fix this. They will be reloaded after setting the new > + * txq numbers for ports to avoid using of not allocated queues by > + * old PMD threads. Newly created threads has no tx queues yet. */ I'm not sure this is easy to maintain. What if we have something like: 1. delete old pmd threads 2. reconfigure txqs 3. add new pmd threads do you this it will be clearer? > + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { > + if (pmd->core_id != NON_PMD_CORE_ID) { > + pmd->need_reload = true; > + } > + } > + } > } > > static void > @@ -3541,6 +3577,29 @@ pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) > } > } > > +static void > +pmd_alloc_static_tx_qid(struct dp_netdev_pmd_thread *pmd) > +{ > + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); > + if (!id_pool_alloc_id(pmd->dp->tx_qid_pool, &pmd->static_tx_qid)) { > + VLOG_ERR("static_tx_qid allocation failed for PMD on core %2d" > + ", numa_id %d.", pmd->core_id, pmd->numa_id); > + OVS_NOT_REACHED(); > + } > + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); > + > + VLOG_DBG("static_tx_qid = %d allocated for PMD thread on core %2d" > + ", numa_id %d.", pmd->static_tx_qid, pmd->core_id, pmd->numa_id); > +} > + > +static void > +pmd_free_static_tx_qid(struct dp_netdev_pmd_thread *pmd) > +{ > + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); > + id_pool_free_id(pmd->dp->tx_qid_pool, pmd->static_tx_qid); > + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); > +} > + > static int > pmd_load_queues_and_ports(struct dp_netdev_pmd_thread *pmd, > struct polled_queue **ppoll_list) > @@ -3586,6 +3645,7 @@ pmd_thread_main(void *f_) > dpdk_set_lcore_id(pmd->core_id); > poll_cnt = pmd_load_queues_and_ports(pmd, &poll_list); > reload: > + pmd_alloc_static_tx_qid(pmd); > emc_cache_init(&pmd->flow_cache); > > /* List port/core affinity */ > @@ -3634,6 +3694,7 @@ reload: > dp_netdev_pmd_reload_done(pmd); > > emc_cache_uninit(&pmd->flow_cache); > + pmd_free_static_tx_qid(pmd); > > if (!exiting) { > goto reload; > @@ -3760,8 +3821,6 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > pmd->numa_id = numa_id; > pmd->need_reload = false; > > - *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp->poll_threads); > - > ovs_refcount_init(&pmd->ref_cnt); > latch_init(&pmd->exit_latch); > pmd->reload_seq = seq_create(); > @@ -3782,6 +3841,7 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > * actual thread created for NON_PMD_CORE_ID. */ > if (core_id == NON_PMD_CORE_ID) { > emc_cache_init(&pmd->flow_cache); > + pmd_alloc_static_tx_qid(pmd); > } > cmap_insert(&dp->poll_threads, CONST_CAST(struct cmap_node *, &pmd->node), > hash_int(core_id, 0)); > @@ -3824,6 +3884,7 @@ dp_netdev_del_pmd(struct dp_netdev *dp, struct dp_netdev_pmd_thread *pmd) > ovs_mutex_lock(&dp->non_pmd_mutex); > emc_cache_uninit(&pmd->flow_cache); > pmd_free_cached_ports(pmd); > + pmd_free_static_tx_qid(pmd); > ovs_mutex_unlock(&dp->non_pmd_mutex); > } else { > latch_set(&pmd->exit_latch); > diff --git a/tests/pmd.at b/tests/pmd.at > index 5686bed..43bdb1b 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -38,7 +38,7 @@ dnl > dnl Whaits for creation of 'n_threads' or at least 1 thread if $1 not > dnl passed. Checking starts from line number 'line' in ovs-vswithd.log . > m4_define([CHECK_PMD_THREADS_CREATED], [ > - PATTERN="Created [[0-9]]* pmd threads on numa node $2" > + PATTERN="There are [[0-9]]* pmd threads on numa node $2" > line_st=$3 > if [[ -z "$line_st" ]] > then > -- > 2.7.4 >
> -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Daniele Di Proietto > Sent: 10 March 2017 04:13 > To: Ilya Maximets <i.maximets@samsung.com> > Cc: dev@openvswitch.org; Heetae Ahn <heetae82.ahn@samsung.com> > Subject: Re: [ovs-dev] [PATCH 2/4] dpif-netdev: Incremental > addition/deletion of PMD threads. > > 2017-02-21 6:49 GMT-08:00 Ilya Maximets <i.maximets@samsung.com>: > > Currently, change of 'pmd-cpu-mask' is very heavy operation. > > It requires destroying of all the PMD threads and creating them back. > > After that, all the threads will sleep until ports' redistribution > > finished. > > > > This patch adds ability to not stop the datapath while adjusting > > number/placement of PMD threads. All not affected threads will forward > > traffic without any additional latencies. > > > > id-pool created for static tx queue ids to keep them sequential in a > > flexible way. non-PMD thread will always have static_tx_qid = 0 as it > > was before. > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks for the patch > > The idea looks good to me. > > I'm still looking at the code, and I have one comment below > Hi Ilya, While reviewing the RFC Patch 4/4 in this series, I ran checkpatch.py over the entire series. The tool returned the warning below for this patch. I don't plan on reviewing this patch, but I thought I was send this as an FYI. # ./checkpatch.py ovs-dev-2-4-dpif-netdev-Incremental-addition-deletion-of-PMD-threads..patch WARNING: Line length is >79-characters long #66 FILE: lib/dpif-netdev.c:1088: /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD threads. */ Lines checked: 272, Warnings: 1, Errors: 0 > > --- > > lib/dpif-netdev.c | 119 > +++++++++++++++++++++++++++++++++++++++++------------- > > tests/pmd.at | 2 +- > > 2 files changed, 91 insertions(+), 30 deletions(-) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > > 30907b7..6e575ab 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -48,6 +48,7 @@ > > #include "fat-rwlock.h" > > #include "flow.h" > > #include "hmapx.h" > > +#include "id-pool.h" > > #include "latch.h" > > #include "netdev.h" > > #include "netdev-vport.h" > > @@ -241,6 +242,9 @@ struct dp_netdev { > > > > /* Stores all 'struct dp_netdev_pmd_thread's. */ > > struct cmap poll_threads; > > + /* id pool for per thread static_tx_qid. */ > > + struct id_pool *tx_qid_pool; > > + struct ovs_mutex tx_qid_pool_mutex; > > > > /* Protects the access of the 'struct dp_netdev_pmd_thread' > > * instance for non-pmd thread. */ @@ -514,7 +518,7 @@ struct > > dp_netdev_pmd_thread { > > /* Queue id used by this pmd thread to send packets on all netdevs if > > * XPS disabled for this netdev. All static_tx_qid's are unique and less > > * than 'cmap_count(dp->poll_threads)'. */ > > - const int static_tx_qid; > > + uint32_t static_tx_qid; > > > > struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and 'tx_ports'. */ > > /* List of rx queues to poll. */ > > @@ -594,6 +598,8 @@ static struct dp_netdev_pmd_thread > *dp_netdev_get_pmd(struct dp_netdev *dp, > > unsigned > > core_id); static struct dp_netdev_pmd_thread * > > dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position > > *pos); > > +static void dp_netdev_del_pmd(struct dp_netdev *dp, > > + struct dp_netdev_pmd_thread *pmd); > > static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp, bool > > non_pmd); static void dp_netdev_pmd_clear_ports(struct > > dp_netdev_pmd_thread *pmd); static void > > dp_netdev_add_port_tx_to_pmd(struct dp_netdev_pmd_thread *pmd, > @@ -1077,10 +1083,17 @@ create_dp_netdev(const char *name, const > struct dpif_class *class, > > atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); > > > > cmap_init(&dp->poll_threads); > > + > > + ovs_mutex_init(&dp->tx_qid_pool_mutex); > > + /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD > threads. */ > > + dp->tx_qid_pool = id_pool_create(0, ovs_numa_get_n_cores() + 1); > > + > > ovs_mutex_init_recursive(&dp->non_pmd_mutex); > > ovsthread_key_create(&dp->per_pmd_key, NULL); > > > > ovs_mutex_lock(&dp->port_mutex); > > + /* non-PMD will be created before all other threads and will > > + * allocate static_tx_qid = 0. */ > > dp_netdev_set_nonpmd(dp); > > > > error = do_add_port(dp, name, > > dpif_netdev_port_open_type(dp->class, > > @@ -1164,6 +1177,9 @@ dp_netdev_free(struct dp_netdev *dp) > > dp_netdev_destroy_all_pmds(dp, true); > > cmap_destroy(&dp->poll_threads); > > > > + ovs_mutex_destroy(&dp->tx_qid_pool_mutex); > > + id_pool_destroy(dp->tx_qid_pool); > > + > > ovs_mutex_destroy(&dp->non_pmd_mutex); > > ovsthread_key_delete(dp->per_pmd_key); > > > > @@ -3175,7 +3191,10 @@ reconfigure_pmd_threads(struct dp_netdev > *dp) > > { > > struct dp_netdev_pmd_thread *pmd; > > struct ovs_numa_dump *pmd_cores; > > - bool changed = false; > > + struct ovs_numa_info_core *core; > > + struct hmapx to_delete = HMAPX_INITIALIZER(&to_delete); > > + struct hmapx_node *node; > > + int created = 0, deleted = 0; > > > > /* The pmd threads should be started only if there's a pmd port in the > > * datapath. If the user didn't provide any "pmd-cpu-mask", we > > start @@ -3188,45 +3207,62 @@ reconfigure_pmd_threads(struct > dp_netdev *dp) > > pmd_cores = > ovs_numa_dump_n_cores_per_numa(NR_PMD_THREADS); > > } > > > > - /* Check for changed configuration */ > > - if (ovs_numa_dump_count(pmd_cores) != cmap_count(&dp- > >poll_threads) - 1) { > > - changed = true; > > - } else { > > - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { > > - if (pmd->core_id != NON_PMD_CORE_ID > > - && !ovs_numa_dump_contains_core(pmd_cores, > > - pmd->numa_id, > > - pmd->core_id)) { > > - changed = true; > > - break; > > - } > > + /* Check for unwanted pmd threads */ > > + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { > > + if (pmd->core_id != NON_PMD_CORE_ID > > + && !ovs_numa_dump_contains_core(pmd_cores, > > + pmd->numa_id, > > + pmd->core_id)) { > > + hmapx_add(&to_delete, pmd); > > } > > } > > + HMAPX_FOR_EACH (node, &to_delete) { > > + pmd = (struct dp_netdev_pmd_thread *) node->data; > > + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d destroyed.", > > + pmd->numa_id, pmd->core_id); > > + dp_netdev_del_pmd(dp, pmd); > > + } > > + deleted = hmapx_count(&to_delete); > > + hmapx_destroy(&to_delete); > > > > - /* Destroy the old and recreate the new pmd threads. We don't > perform an > > - * incremental update because we would have to adjust 'static_tx_qid'. > */ > > - if (changed) { > > - struct ovs_numa_info_core *core; > > - struct ovs_numa_info_numa *numa; > > - > > - /* Do not destroy the non pmd thread. */ > > - dp_netdev_destroy_all_pmds(dp, false); > > - FOR_EACH_CORE_ON_DUMP (core, pmd_cores) { > > - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd); > > - > > + /* Check for required new pmd threads */ > > + FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { > > + pmd = dp_netdev_get_pmd(dp, core->core_id); > > + if (!pmd) { > > + pmd = xzalloc(sizeof *pmd); > > dp_netdev_configure_pmd(pmd, dp, core->core_id, > > core->numa_id); > > - > > pmd->thread = ovs_thread_create("pmd", pmd_thread_main, > > pmd); > > + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d created.", > > + pmd->numa_id, pmd->core_id); > > + created++; > > + } else { > > + dp_netdev_pmd_unref(pmd); > > } > > + } > > + > > + if (deleted || created) { > > + struct ovs_numa_info_numa *numa; > > > > /* Log the number of pmd threads per numa node. */ > > FOR_EACH_NUMA_ON_DUMP (numa, pmd_cores) { > > - VLOG_INFO("Created %"PRIuSIZE" pmd threads on numa node %d", > > + VLOG_INFO("There are %"PRIuSIZE" pmd threads on numa node > > + %d", > > numa->n_cores, numa->numa_id); > > } > > } > > > > ovs_numa_dump_destroy(pmd_cores); > > + > > + if (deleted > created) { > > + /* Static tx qids are not sequential now. Mark all threads for > > + * reload to fix this. They will be reloaded after setting the new > > + * txq numbers for ports to avoid using of not allocated queues by > > + * old PMD threads. Newly created threads has no tx queues > > + yet. */ > > I'm not sure this is easy to maintain. > > What if we have something like: > > 1. delete old pmd threads > 2. reconfigure txqs > 3. add new pmd threads > > do you this it will be clearer? > > > + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { > > + if (pmd->core_id != NON_PMD_CORE_ID) { > > + pmd->need_reload = true; > > + } > > + } > > + } > > } > > > > static void > > @@ -3541,6 +3577,29 @@ pmd_load_cached_ports(struct > dp_netdev_pmd_thread *pmd) > > } > > } > > > > +static void > > +pmd_alloc_static_tx_qid(struct dp_netdev_pmd_thread *pmd) { > > + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); > > + if (!id_pool_alloc_id(pmd->dp->tx_qid_pool, &pmd->static_tx_qid)) { > > + VLOG_ERR("static_tx_qid allocation failed for PMD on core %2d" > > + ", numa_id %d.", pmd->core_id, pmd->numa_id); > > + OVS_NOT_REACHED(); > > + } > > + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); > > + > > + VLOG_DBG("static_tx_qid = %d allocated for PMD thread on core %2d" > > + ", numa_id %d.", pmd->static_tx_qid, pmd->core_id, > > +pmd->numa_id); } > > + > > +static void > > +pmd_free_static_tx_qid(struct dp_netdev_pmd_thread *pmd) { > > + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); > > + id_pool_free_id(pmd->dp->tx_qid_pool, pmd->static_tx_qid); > > + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); > > +} > > + > > static int > > pmd_load_queues_and_ports(struct dp_netdev_pmd_thread *pmd, > > struct polled_queue **ppoll_list) @@ > > -3586,6 +3645,7 @@ pmd_thread_main(void *f_) > > dpdk_set_lcore_id(pmd->core_id); > > poll_cnt = pmd_load_queues_and_ports(pmd, &poll_list); > > reload: > > + pmd_alloc_static_tx_qid(pmd); > > emc_cache_init(&pmd->flow_cache); > > > > /* List port/core affinity */ > > @@ -3634,6 +3694,7 @@ reload: > > dp_netdev_pmd_reload_done(pmd); > > > > emc_cache_uninit(&pmd->flow_cache); > > + pmd_free_static_tx_qid(pmd); > > > > if (!exiting) { > > goto reload; > > @@ -3760,8 +3821,6 @@ dp_netdev_configure_pmd(struct > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > pmd->numa_id = numa_id; > > pmd->need_reload = false; > > > > - *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp- > >poll_threads); > > - > > ovs_refcount_init(&pmd->ref_cnt); > > latch_init(&pmd->exit_latch); > > pmd->reload_seq = seq_create(); > > @@ -3782,6 +3841,7 @@ dp_netdev_configure_pmd(struct > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > * actual thread created for NON_PMD_CORE_ID. */ > > if (core_id == NON_PMD_CORE_ID) { > > emc_cache_init(&pmd->flow_cache); > > + pmd_alloc_static_tx_qid(pmd); > > } > > cmap_insert(&dp->poll_threads, CONST_CAST(struct cmap_node *, > &pmd->node), > > hash_int(core_id, 0)); @@ -3824,6 +3884,7 @@ > > dp_netdev_del_pmd(struct dp_netdev *dp, struct > dp_netdev_pmd_thread *pmd) > > ovs_mutex_lock(&dp->non_pmd_mutex); > > emc_cache_uninit(&pmd->flow_cache); > > pmd_free_cached_ports(pmd); > > + pmd_free_static_tx_qid(pmd); > > ovs_mutex_unlock(&dp->non_pmd_mutex); > > } else { > > latch_set(&pmd->exit_latch); > > diff --git a/tests/pmd.at b/tests/pmd.at index 5686bed..43bdb1b 100644 > > --- a/tests/pmd.at > > +++ b/tests/pmd.at > > @@ -38,7 +38,7 @@ dnl > > dnl Whaits for creation of 'n_threads' or at least 1 thread if $1 not > > dnl passed. Checking starts from line number 'line' in ovs-vswithd.log . > > m4_define([CHECK_PMD_THREADS_CREATED], [ > > - PATTERN="Created [[0-9]]* pmd threads on numa node $2" > > + PATTERN="There are [[0-9]]* pmd threads on numa node $2" > > line_st=$3 > > if [[ -z "$line_st" ]] > > then > > -- > > 2.7.4 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 29.05.2017 18:41, Ferriter, Cian wrote: >> -----Original Message----- >> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >> bounces@openvswitch.org] On Behalf Of Daniele Di Proietto >> Sent: 10 March 2017 04:13 >> To: Ilya Maximets <i.maximets@samsung.com> >> Cc: dev@openvswitch.org; Heetae Ahn <heetae82.ahn@samsung.com> >> Subject: Re: [ovs-dev] [PATCH 2/4] dpif-netdev: Incremental >> addition/deletion of PMD threads. >> >> 2017-02-21 6:49 GMT-08:00 Ilya Maximets <i.maximets@samsung.com>: >>> Currently, change of 'pmd-cpu-mask' is very heavy operation. >>> It requires destroying of all the PMD threads and creating them back. >>> After that, all the threads will sleep until ports' redistribution >>> finished. >>> >>> This patch adds ability to not stop the datapath while adjusting >>> number/placement of PMD threads. All not affected threads will forward >>> traffic without any additional latencies. >>> >>> id-pool created for static tx queue ids to keep them sequential in a >>> flexible way. non-PMD thread will always have static_tx_qid = 0 as it >>> was before. >>> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> >> Thanks for the patch >> >> The idea looks good to me. >> >> I'm still looking at the code, and I have one comment below >> > > Hi Ilya, > > While reviewing the RFC Patch 4/4 in this series, I ran checkpatch.py over the entire series. The tool returned the warning below for this patch. I don't plan on reviewing this patch, but I thought I was send this as an FYI. > > # ./checkpatch.py ovs-dev-2-4-dpif-netdev-Incremental-addition-deletion-of-PMD-threads..patch > WARNING: Line length is >79-characters long > #66 FILE: lib/dpif-netdev.c:1088: > /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD threads. */ > > Lines checked: 272, Warnings: 1, Errors: 0 Thanks. I'll fix it. Best regards, Ilya Maximets. >>> --- >>> lib/dpif-netdev.c | 119 >> +++++++++++++++++++++++++++++++++++++++++------------- >>> tests/pmd.at | 2 +- >>> 2 files changed, 91 insertions(+), 30 deletions(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index >>> 30907b7..6e575ab 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -48,6 +48,7 @@ >>> #include "fat-rwlock.h" >>> #include "flow.h" >>> #include "hmapx.h" >>> +#include "id-pool.h" >>> #include "latch.h" >>> #include "netdev.h" >>> #include "netdev-vport.h" >>> @@ -241,6 +242,9 @@ struct dp_netdev { >>> >>> /* Stores all 'struct dp_netdev_pmd_thread's. */ >>> struct cmap poll_threads; >>> + /* id pool for per thread static_tx_qid. */ >>> + struct id_pool *tx_qid_pool; >>> + struct ovs_mutex tx_qid_pool_mutex; >>> >>> /* Protects the access of the 'struct dp_netdev_pmd_thread' >>> * instance for non-pmd thread. */ @@ -514,7 +518,7 @@ struct >>> dp_netdev_pmd_thread { >>> /* Queue id used by this pmd thread to send packets on all netdevs if >>> * XPS disabled for this netdev. All static_tx_qid's are unique and less >>> * than 'cmap_count(dp->poll_threads)'. */ >>> - const int static_tx_qid; >>> + uint32_t static_tx_qid; >>> >>> struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and 'tx_ports'. */ >>> /* List of rx queues to poll. */ >>> @@ -594,6 +598,8 @@ static struct dp_netdev_pmd_thread >> *dp_netdev_get_pmd(struct dp_netdev *dp, >>> unsigned >>> core_id); static struct dp_netdev_pmd_thread * >>> dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position >>> *pos); >>> +static void dp_netdev_del_pmd(struct dp_netdev *dp, >>> + struct dp_netdev_pmd_thread *pmd); >>> static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp, bool >>> non_pmd); static void dp_netdev_pmd_clear_ports(struct >>> dp_netdev_pmd_thread *pmd); static void >>> dp_netdev_add_port_tx_to_pmd(struct dp_netdev_pmd_thread *pmd, >> @@ -1077,10 +1083,17 @@ create_dp_netdev(const char *name, const >> struct dpif_class *class, >>> atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); >>> >>> cmap_init(&dp->poll_threads); >>> + >>> + ovs_mutex_init(&dp->tx_qid_pool_mutex); >>> + /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD >> threads. */ >>> + dp->tx_qid_pool = id_pool_create(0, ovs_numa_get_n_cores() + 1); >>> + >>> ovs_mutex_init_recursive(&dp->non_pmd_mutex); >>> ovsthread_key_create(&dp->per_pmd_key, NULL); >>> >>> ovs_mutex_lock(&dp->port_mutex); >>> + /* non-PMD will be created before all other threads and will >>> + * allocate static_tx_qid = 0. */ >>> dp_netdev_set_nonpmd(dp); >>> >>> error = do_add_port(dp, name, >>> dpif_netdev_port_open_type(dp->class, >>> @@ -1164,6 +1177,9 @@ dp_netdev_free(struct dp_netdev *dp) >>> dp_netdev_destroy_all_pmds(dp, true); >>> cmap_destroy(&dp->poll_threads); >>> >>> + ovs_mutex_destroy(&dp->tx_qid_pool_mutex); >>> + id_pool_destroy(dp->tx_qid_pool); >>> + >>> ovs_mutex_destroy(&dp->non_pmd_mutex); >>> ovsthread_key_delete(dp->per_pmd_key); >>> >>> @@ -3175,7 +3191,10 @@ reconfigure_pmd_threads(struct dp_netdev >> *dp) >>> { >>> struct dp_netdev_pmd_thread *pmd; >>> struct ovs_numa_dump *pmd_cores; >>> - bool changed = false; >>> + struct ovs_numa_info_core *core; >>> + struct hmapx to_delete = HMAPX_INITIALIZER(&to_delete); >>> + struct hmapx_node *node; >>> + int created = 0, deleted = 0; >>> >>> /* The pmd threads should be started only if there's a pmd port in the >>> * datapath. If the user didn't provide any "pmd-cpu-mask", we >>> start @@ -3188,45 +3207,62 @@ reconfigure_pmd_threads(struct >> dp_netdev *dp) >>> pmd_cores = >> ovs_numa_dump_n_cores_per_numa(NR_PMD_THREADS); >>> } >>> >>> - /* Check for changed configuration */ >>> - if (ovs_numa_dump_count(pmd_cores) != cmap_count(&dp- >>> poll_threads) - 1) { >>> - changed = true; >>> - } else { >>> - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { >>> - if (pmd->core_id != NON_PMD_CORE_ID >>> - && !ovs_numa_dump_contains_core(pmd_cores, >>> - pmd->numa_id, >>> - pmd->core_id)) { >>> - changed = true; >>> - break; >>> - } >>> + /* Check for unwanted pmd threads */ >>> + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { >>> + if (pmd->core_id != NON_PMD_CORE_ID >>> + && !ovs_numa_dump_contains_core(pmd_cores, >>> + pmd->numa_id, >>> + pmd->core_id)) { >>> + hmapx_add(&to_delete, pmd); >>> } >>> } >>> + HMAPX_FOR_EACH (node, &to_delete) { >>> + pmd = (struct dp_netdev_pmd_thread *) node->data; >>> + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d destroyed.", >>> + pmd->numa_id, pmd->core_id); >>> + dp_netdev_del_pmd(dp, pmd); >>> + } >>> + deleted = hmapx_count(&to_delete); >>> + hmapx_destroy(&to_delete); >>> >>> - /* Destroy the old and recreate the new pmd threads. We don't >> perform an >>> - * incremental update because we would have to adjust 'static_tx_qid'. >> */ >>> - if (changed) { >>> - struct ovs_numa_info_core *core; >>> - struct ovs_numa_info_numa *numa; >>> - >>> - /* Do not destroy the non pmd thread. */ >>> - dp_netdev_destroy_all_pmds(dp, false); >>> - FOR_EACH_CORE_ON_DUMP (core, pmd_cores) { >>> - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd); >>> - >>> + /* Check for required new pmd threads */ >>> + FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { >>> + pmd = dp_netdev_get_pmd(dp, core->core_id); >>> + if (!pmd) { >>> + pmd = xzalloc(sizeof *pmd); >>> dp_netdev_configure_pmd(pmd, dp, core->core_id, >>> core->numa_id); >>> - >>> pmd->thread = ovs_thread_create("pmd", pmd_thread_main, >>> pmd); >>> + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d created.", >>> + pmd->numa_id, pmd->core_id); >>> + created++; >>> + } else { >>> + dp_netdev_pmd_unref(pmd); >>> } >>> + } >>> + >>> + if (deleted || created) { >>> + struct ovs_numa_info_numa *numa; >>> >>> /* Log the number of pmd threads per numa node. */ >>> FOR_EACH_NUMA_ON_DUMP (numa, pmd_cores) { >>> - VLOG_INFO("Created %"PRIuSIZE" pmd threads on numa node %d", >>> + VLOG_INFO("There are %"PRIuSIZE" pmd threads on numa node >>> + %d", >>> numa->n_cores, numa->numa_id); >>> } >>> } >>> >>> ovs_numa_dump_destroy(pmd_cores); >>> + >>> + if (deleted > created) { >>> + /* Static tx qids are not sequential now. Mark all threads for >>> + * reload to fix this. They will be reloaded after setting the new >>> + * txq numbers for ports to avoid using of not allocated queues by >>> + * old PMD threads. Newly created threads has no tx queues >>> + yet. */ >> >> I'm not sure this is easy to maintain. >> >> What if we have something like: >> >> 1. delete old pmd threads >> 2. reconfigure txqs >> 3. add new pmd threads >> >> do you this it will be clearer? >> >>> + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { >>> + if (pmd->core_id != NON_PMD_CORE_ID) { >>> + pmd->need_reload = true; >>> + } >>> + } >>> + } >>> } >>> >>> static void >>> @@ -3541,6 +3577,29 @@ pmd_load_cached_ports(struct >> dp_netdev_pmd_thread *pmd) >>> } >>> } >>> >>> +static void >>> +pmd_alloc_static_tx_qid(struct dp_netdev_pmd_thread *pmd) { >>> + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); >>> + if (!id_pool_alloc_id(pmd->dp->tx_qid_pool, &pmd->static_tx_qid)) { >>> + VLOG_ERR("static_tx_qid allocation failed for PMD on core %2d" >>> + ", numa_id %d.", pmd->core_id, pmd->numa_id); >>> + OVS_NOT_REACHED(); >>> + } >>> + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); >>> + >>> + VLOG_DBG("static_tx_qid = %d allocated for PMD thread on core %2d" >>> + ", numa_id %d.", pmd->static_tx_qid, pmd->core_id, >>> +pmd->numa_id); } >>> + >>> +static void >>> +pmd_free_static_tx_qid(struct dp_netdev_pmd_thread *pmd) { >>> + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); >>> + id_pool_free_id(pmd->dp->tx_qid_pool, pmd->static_tx_qid); >>> + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); >>> +} >>> + >>> static int >>> pmd_load_queues_and_ports(struct dp_netdev_pmd_thread *pmd, >>> struct polled_queue **ppoll_list) @@ >>> -3586,6 +3645,7 @@ pmd_thread_main(void *f_) >>> dpdk_set_lcore_id(pmd->core_id); >>> poll_cnt = pmd_load_queues_and_ports(pmd, &poll_list); >>> reload: >>> + pmd_alloc_static_tx_qid(pmd); >>> emc_cache_init(&pmd->flow_cache); >>> >>> /* List port/core affinity */ >>> @@ -3634,6 +3694,7 @@ reload: >>> dp_netdev_pmd_reload_done(pmd); >>> >>> emc_cache_uninit(&pmd->flow_cache); >>> + pmd_free_static_tx_qid(pmd); >>> >>> if (!exiting) { >>> goto reload; >>> @@ -3760,8 +3821,6 @@ dp_netdev_configure_pmd(struct >> dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, >>> pmd->numa_id = numa_id; >>> pmd->need_reload = false; >>> >>> - *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp- >>> poll_threads); >>> - >>> ovs_refcount_init(&pmd->ref_cnt); >>> latch_init(&pmd->exit_latch); >>> pmd->reload_seq = seq_create(); >>> @@ -3782,6 +3841,7 @@ dp_netdev_configure_pmd(struct >> dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, >>> * actual thread created for NON_PMD_CORE_ID. */ >>> if (core_id == NON_PMD_CORE_ID) { >>> emc_cache_init(&pmd->flow_cache); >>> + pmd_alloc_static_tx_qid(pmd); >>> } >>> cmap_insert(&dp->poll_threads, CONST_CAST(struct cmap_node *, >> &pmd->node), >>> hash_int(core_id, 0)); @@ -3824,6 +3884,7 @@ >>> dp_netdev_del_pmd(struct dp_netdev *dp, struct >> dp_netdev_pmd_thread *pmd) >>> ovs_mutex_lock(&dp->non_pmd_mutex); >>> emc_cache_uninit(&pmd->flow_cache); >>> pmd_free_cached_ports(pmd); >>> + pmd_free_static_tx_qid(pmd); >>> ovs_mutex_unlock(&dp->non_pmd_mutex); >>> } else { >>> latch_set(&pmd->exit_latch); >>> diff --git a/tests/pmd.at b/tests/pmd.at index 5686bed..43bdb1b 100644 >>> --- a/tests/pmd.at >>> +++ b/tests/pmd.at >>> @@ -38,7 +38,7 @@ dnl >>> dnl Whaits for creation of 'n_threads' or at least 1 thread if $1 not >>> dnl passed. Checking starts from line number 'line' in ovs-vswithd.log . >>> m4_define([CHECK_PMD_THREADS_CREATED], [ >>> - PATTERN="Created [[0-9]]* pmd threads on numa node $2" >>> + PATTERN="There are [[0-9]]* pmd threads on numa node $2" >>> line_st=$3 >>> if [[ -z "$line_st" ]] >>> then >>> -- >>> 2.7.4 >>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 10.03.2017 07:12, Daniele Di Proietto wrote: > 2017-02-21 6:49 GMT-08:00 Ilya Maximets <i.maximets@samsung.com>: >> Currently, change of 'pmd-cpu-mask' is very heavy operation. >> It requires destroying of all the PMD threads and creating >> them back. After that, all the threads will sleep until >> ports' redistribution finished. >> >> This patch adds ability to not stop the datapath while >> adjusting number/placement of PMD threads. All not affected >> threads will forward traffic without any additional latencies. >> >> id-pool created for static tx queue ids to keep them sequential >> in a flexible way. non-PMD thread will always have >> static_tx_qid = 0 as it was before. >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks for the patch > > The idea looks good to me. > > I'm still looking at the code, and I have one comment below > >> --- >> lib/dpif-netdev.c | 119 +++++++++++++++++++++++++++++++++++++++++------------- >> tests/pmd.at | 2 +- >> 2 files changed, 91 insertions(+), 30 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 30907b7..6e575ab 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -48,6 +48,7 @@ >> #include "fat-rwlock.h" >> #include "flow.h" >> #include "hmapx.h" >> +#include "id-pool.h" >> #include "latch.h" >> #include "netdev.h" >> #include "netdev-vport.h" >> @@ -241,6 +242,9 @@ struct dp_netdev { >> >> /* Stores all 'struct dp_netdev_pmd_thread's. */ >> struct cmap poll_threads; >> + /* id pool for per thread static_tx_qid. */ >> + struct id_pool *tx_qid_pool; >> + struct ovs_mutex tx_qid_pool_mutex; >> >> /* Protects the access of the 'struct dp_netdev_pmd_thread' >> * instance for non-pmd thread. */ >> @@ -514,7 +518,7 @@ struct dp_netdev_pmd_thread { >> /* Queue id used by this pmd thread to send packets on all netdevs if >> * XPS disabled for this netdev. All static_tx_qid's are unique and less >> * than 'cmap_count(dp->poll_threads)'. */ >> - const int static_tx_qid; >> + uint32_t static_tx_qid; >> >> struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and 'tx_ports'. */ >> /* List of rx queues to poll. */ >> @@ -594,6 +598,8 @@ static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp, >> unsigned core_id); >> static struct dp_netdev_pmd_thread * >> dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position *pos); >> +static void dp_netdev_del_pmd(struct dp_netdev *dp, >> + struct dp_netdev_pmd_thread *pmd); >> static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp, bool non_pmd); >> static void dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread *pmd); >> static void dp_netdev_add_port_tx_to_pmd(struct dp_netdev_pmd_thread *pmd, >> @@ -1077,10 +1083,17 @@ create_dp_netdev(const char *name, const struct dpif_class *class, >> atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); >> >> cmap_init(&dp->poll_threads); >> + >> + ovs_mutex_init(&dp->tx_qid_pool_mutex); >> + /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD threads. */ >> + dp->tx_qid_pool = id_pool_create(0, ovs_numa_get_n_cores() + 1); >> + >> ovs_mutex_init_recursive(&dp->non_pmd_mutex); >> ovsthread_key_create(&dp->per_pmd_key, NULL); >> >> ovs_mutex_lock(&dp->port_mutex); >> + /* non-PMD will be created before all other threads and will >> + * allocate static_tx_qid = 0. */ >> dp_netdev_set_nonpmd(dp); >> >> error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class, >> @@ -1164,6 +1177,9 @@ dp_netdev_free(struct dp_netdev *dp) >> dp_netdev_destroy_all_pmds(dp, true); >> cmap_destroy(&dp->poll_threads); >> >> + ovs_mutex_destroy(&dp->tx_qid_pool_mutex); >> + id_pool_destroy(dp->tx_qid_pool); >> + >> ovs_mutex_destroy(&dp->non_pmd_mutex); >> ovsthread_key_delete(dp->per_pmd_key); >> >> @@ -3175,7 +3191,10 @@ reconfigure_pmd_threads(struct dp_netdev *dp) >> { >> struct dp_netdev_pmd_thread *pmd; >> struct ovs_numa_dump *pmd_cores; >> - bool changed = false; >> + struct ovs_numa_info_core *core; >> + struct hmapx to_delete = HMAPX_INITIALIZER(&to_delete); >> + struct hmapx_node *node; >> + int created = 0, deleted = 0; >> >> /* The pmd threads should be started only if there's a pmd port in the >> * datapath. If the user didn't provide any "pmd-cpu-mask", we start >> @@ -3188,45 +3207,62 @@ reconfigure_pmd_threads(struct dp_netdev *dp) >> pmd_cores = ovs_numa_dump_n_cores_per_numa(NR_PMD_THREADS); >> } >> >> - /* Check for changed configuration */ >> - if (ovs_numa_dump_count(pmd_cores) != cmap_count(&dp->poll_threads) - 1) { >> - changed = true; >> - } else { >> - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { >> - if (pmd->core_id != NON_PMD_CORE_ID >> - && !ovs_numa_dump_contains_core(pmd_cores, >> - pmd->numa_id, >> - pmd->core_id)) { >> - changed = true; >> - break; >> - } >> + /* Check for unwanted pmd threads */ >> + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { >> + if (pmd->core_id != NON_PMD_CORE_ID >> + && !ovs_numa_dump_contains_core(pmd_cores, >> + pmd->numa_id, >> + pmd->core_id)) { >> + hmapx_add(&to_delete, pmd); >> } >> } >> + HMAPX_FOR_EACH (node, &to_delete) { >> + pmd = (struct dp_netdev_pmd_thread *) node->data; >> + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d destroyed.", >> + pmd->numa_id, pmd->core_id); >> + dp_netdev_del_pmd(dp, pmd); >> + } >> + deleted = hmapx_count(&to_delete); >> + hmapx_destroy(&to_delete); >> >> - /* Destroy the old and recreate the new pmd threads. We don't perform an >> - * incremental update because we would have to adjust 'static_tx_qid'. */ >> - if (changed) { >> - struct ovs_numa_info_core *core; >> - struct ovs_numa_info_numa *numa; >> - >> - /* Do not destroy the non pmd thread. */ >> - dp_netdev_destroy_all_pmds(dp, false); >> - FOR_EACH_CORE_ON_DUMP (core, pmd_cores) { >> - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd); >> - >> + /* Check for required new pmd threads */ >> + FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { >> + pmd = dp_netdev_get_pmd(dp, core->core_id); >> + if (!pmd) { >> + pmd = xzalloc(sizeof *pmd); >> dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id); >> - >> pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd); >> + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d created.", >> + pmd->numa_id, pmd->core_id); >> + created++; >> + } else { >> + dp_netdev_pmd_unref(pmd); >> } >> + } >> + >> + if (deleted || created) { >> + struct ovs_numa_info_numa *numa; >> >> /* Log the number of pmd threads per numa node. */ >> FOR_EACH_NUMA_ON_DUMP (numa, pmd_cores) { >> - VLOG_INFO("Created %"PRIuSIZE" pmd threads on numa node %d", >> + VLOG_INFO("There are %"PRIuSIZE" pmd threads on numa node %d", >> numa->n_cores, numa->numa_id); >> } >> } >> >> ovs_numa_dump_destroy(pmd_cores); >> + >> + if (deleted > created) { >> + /* Static tx qids are not sequential now. Mark all threads for >> + * reload to fix this. They will be reloaded after setting the new >> + * txq numbers for ports to avoid using of not allocated queues by >> + * old PMD threads. Newly created threads has no tx queues yet. */ > > I'm not sure this is easy to maintain. > > What if we have something like: > > 1. delete old pmd threads > 2. reconfigure txqs > 3. add new pmd threads > > do you this it will be clearer? Yes. I think it's reasonable. One thing is that we need to reconfigure txqs only if resulting number of PMD threads will be less then before. In other cases all the gaps in allocated 'static_tx_qid's will be filled by newly created PMD threads. > >> + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { >> + if (pmd->core_id != NON_PMD_CORE_ID) { >> + pmd->need_reload = true; >> + } >> + } >> + } >> } >> >> static void >> @@ -3541,6 +3577,29 @@ pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) >> } >> } >> >> +static void >> +pmd_alloc_static_tx_qid(struct dp_netdev_pmd_thread *pmd) >> +{ >> + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); >> + if (!id_pool_alloc_id(pmd->dp->tx_qid_pool, &pmd->static_tx_qid)) { >> + VLOG_ERR("static_tx_qid allocation failed for PMD on core %2d" >> + ", numa_id %d.", pmd->core_id, pmd->numa_id); >> + OVS_NOT_REACHED(); >> + } >> + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); >> + >> + VLOG_DBG("static_tx_qid = %d allocated for PMD thread on core %2d" >> + ", numa_id %d.", pmd->static_tx_qid, pmd->core_id, pmd->numa_id); >> +} >> + >> +static void >> +pmd_free_static_tx_qid(struct dp_netdev_pmd_thread *pmd) >> +{ >> + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); >> + id_pool_free_id(pmd->dp->tx_qid_pool, pmd->static_tx_qid); >> + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); >> +} >> + >> static int >> pmd_load_queues_and_ports(struct dp_netdev_pmd_thread *pmd, >> struct polled_queue **ppoll_list) >> @@ -3586,6 +3645,7 @@ pmd_thread_main(void *f_) >> dpdk_set_lcore_id(pmd->core_id); >> poll_cnt = pmd_load_queues_and_ports(pmd, &poll_list); >> reload: >> + pmd_alloc_static_tx_qid(pmd); >> emc_cache_init(&pmd->flow_cache); >> >> /* List port/core affinity */ >> @@ -3634,6 +3694,7 @@ reload: >> dp_netdev_pmd_reload_done(pmd); >> >> emc_cache_uninit(&pmd->flow_cache); >> + pmd_free_static_tx_qid(pmd); >> >> if (!exiting) { >> goto reload; >> @@ -3760,8 +3821,6 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, >> pmd->numa_id = numa_id; >> pmd->need_reload = false; >> >> - *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp->poll_threads); >> - >> ovs_refcount_init(&pmd->ref_cnt); >> latch_init(&pmd->exit_latch); >> pmd->reload_seq = seq_create(); >> @@ -3782,6 +3841,7 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, >> * actual thread created for NON_PMD_CORE_ID. */ >> if (core_id == NON_PMD_CORE_ID) { >> emc_cache_init(&pmd->flow_cache); >> + pmd_alloc_static_tx_qid(pmd); >> } >> cmap_insert(&dp->poll_threads, CONST_CAST(struct cmap_node *, &pmd->node), >> hash_int(core_id, 0)); >> @@ -3824,6 +3884,7 @@ dp_netdev_del_pmd(struct dp_netdev *dp, struct dp_netdev_pmd_thread *pmd) >> ovs_mutex_lock(&dp->non_pmd_mutex); >> emc_cache_uninit(&pmd->flow_cache); >> pmd_free_cached_ports(pmd); >> + pmd_free_static_tx_qid(pmd); >> ovs_mutex_unlock(&dp->non_pmd_mutex); >> } else { >> latch_set(&pmd->exit_latch); >> diff --git a/tests/pmd.at b/tests/pmd.at >> index 5686bed..43bdb1b 100644 >> --- a/tests/pmd.at >> +++ b/tests/pmd.at >> @@ -38,7 +38,7 @@ dnl >> dnl Whaits for creation of 'n_threads' or at least 1 thread if $1 not >> dnl passed. Checking starts from line number 'line' in ovs-vswithd.log . >> m4_define([CHECK_PMD_THREADS_CREATED], [ >> - PATTERN="Created [[0-9]]* pmd threads on numa node $2" >> + PATTERN="There are [[0-9]]* pmd threads on numa node $2" >> line_st=$3 >> if [[ -z "$line_st" ]] >> then >> -- >> 2.7.4 >> > > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 30907b7..6e575ab 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -48,6 +48,7 @@ #include "fat-rwlock.h" #include "flow.h" #include "hmapx.h" +#include "id-pool.h" #include "latch.h" #include "netdev.h" #include "netdev-vport.h" @@ -241,6 +242,9 @@ struct dp_netdev { /* Stores all 'struct dp_netdev_pmd_thread's. */ struct cmap poll_threads; + /* id pool for per thread static_tx_qid. */ + struct id_pool *tx_qid_pool; + struct ovs_mutex tx_qid_pool_mutex; /* Protects the access of the 'struct dp_netdev_pmd_thread' * instance for non-pmd thread. */ @@ -514,7 +518,7 @@ struct dp_netdev_pmd_thread { /* Queue id used by this pmd thread to send packets on all netdevs if * XPS disabled for this netdev. All static_tx_qid's are unique and less * than 'cmap_count(dp->poll_threads)'. */ - const int static_tx_qid; + uint32_t static_tx_qid; struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and 'tx_ports'. */ /* List of rx queues to poll. */ @@ -594,6 +598,8 @@ static struct dp_netdev_pmd_thread *dp_netdev_get_pmd(struct dp_netdev *dp, unsigned core_id); static struct dp_netdev_pmd_thread * dp_netdev_pmd_get_next(struct dp_netdev *dp, struct cmap_position *pos); +static void dp_netdev_del_pmd(struct dp_netdev *dp, + struct dp_netdev_pmd_thread *pmd); static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp, bool non_pmd); static void dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread *pmd); static void dp_netdev_add_port_tx_to_pmd(struct dp_netdev_pmd_thread *pmd, @@ -1077,10 +1083,17 @@ create_dp_netdev(const char *name, const struct dpif_class *class, atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); cmap_init(&dp->poll_threads); + + ovs_mutex_init(&dp->tx_qid_pool_mutex); + /* We need 1 Tx queue for each possible cpu core + 1 for non-PMD threads. */ + dp->tx_qid_pool = id_pool_create(0, ovs_numa_get_n_cores() + 1); + ovs_mutex_init_recursive(&dp->non_pmd_mutex); ovsthread_key_create(&dp->per_pmd_key, NULL); ovs_mutex_lock(&dp->port_mutex); + /* non-PMD will be created before all other threads and will + * allocate static_tx_qid = 0. */ dp_netdev_set_nonpmd(dp); error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class, @@ -1164,6 +1177,9 @@ dp_netdev_free(struct dp_netdev *dp) dp_netdev_destroy_all_pmds(dp, true); cmap_destroy(&dp->poll_threads); + ovs_mutex_destroy(&dp->tx_qid_pool_mutex); + id_pool_destroy(dp->tx_qid_pool); + ovs_mutex_destroy(&dp->non_pmd_mutex); ovsthread_key_delete(dp->per_pmd_key); @@ -3175,7 +3191,10 @@ reconfigure_pmd_threads(struct dp_netdev *dp) { struct dp_netdev_pmd_thread *pmd; struct ovs_numa_dump *pmd_cores; - bool changed = false; + struct ovs_numa_info_core *core; + struct hmapx to_delete = HMAPX_INITIALIZER(&to_delete); + struct hmapx_node *node; + int created = 0, deleted = 0; /* The pmd threads should be started only if there's a pmd port in the * datapath. If the user didn't provide any "pmd-cpu-mask", we start @@ -3188,45 +3207,62 @@ reconfigure_pmd_threads(struct dp_netdev *dp) pmd_cores = ovs_numa_dump_n_cores_per_numa(NR_PMD_THREADS); } - /* Check for changed configuration */ - if (ovs_numa_dump_count(pmd_cores) != cmap_count(&dp->poll_threads) - 1) { - changed = true; - } else { - CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { - if (pmd->core_id != NON_PMD_CORE_ID - && !ovs_numa_dump_contains_core(pmd_cores, - pmd->numa_id, - pmd->core_id)) { - changed = true; - break; - } + /* Check for unwanted pmd threads */ + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { + if (pmd->core_id != NON_PMD_CORE_ID + && !ovs_numa_dump_contains_core(pmd_cores, + pmd->numa_id, + pmd->core_id)) { + hmapx_add(&to_delete, pmd); } } + HMAPX_FOR_EACH (node, &to_delete) { + pmd = (struct dp_netdev_pmd_thread *) node->data; + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d destroyed.", + pmd->numa_id, pmd->core_id); + dp_netdev_del_pmd(dp, pmd); + } + deleted = hmapx_count(&to_delete); + hmapx_destroy(&to_delete); - /* Destroy the old and recreate the new pmd threads. We don't perform an - * incremental update because we would have to adjust 'static_tx_qid'. */ - if (changed) { - struct ovs_numa_info_core *core; - struct ovs_numa_info_numa *numa; - - /* Do not destroy the non pmd thread. */ - dp_netdev_destroy_all_pmds(dp, false); - FOR_EACH_CORE_ON_DUMP (core, pmd_cores) { - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd); - + /* Check for required new pmd threads */ + FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { + pmd = dp_netdev_get_pmd(dp, core->core_id); + if (!pmd) { + pmd = xzalloc(sizeof *pmd); dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id); - pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd); + VLOG_INFO("PMD thread on numa_id: %d, core id: %2d created.", + pmd->numa_id, pmd->core_id); + created++; + } else { + dp_netdev_pmd_unref(pmd); } + } + + if (deleted || created) { + struct ovs_numa_info_numa *numa; /* Log the number of pmd threads per numa node. */ FOR_EACH_NUMA_ON_DUMP (numa, pmd_cores) { - VLOG_INFO("Created %"PRIuSIZE" pmd threads on numa node %d", + VLOG_INFO("There are %"PRIuSIZE" pmd threads on numa node %d", numa->n_cores, numa->numa_id); } } ovs_numa_dump_destroy(pmd_cores); + + if (deleted > created) { + /* Static tx qids are not sequential now. Mark all threads for + * reload to fix this. They will be reloaded after setting the new + * txq numbers for ports to avoid using of not allocated queues by + * old PMD threads. Newly created threads has no tx queues yet. */ + CMAP_FOR_EACH(pmd, node, &dp->poll_threads) { + if (pmd->core_id != NON_PMD_CORE_ID) { + pmd->need_reload = true; + } + } + } } static void @@ -3541,6 +3577,29 @@ pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) } } +static void +pmd_alloc_static_tx_qid(struct dp_netdev_pmd_thread *pmd) +{ + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); + if (!id_pool_alloc_id(pmd->dp->tx_qid_pool, &pmd->static_tx_qid)) { + VLOG_ERR("static_tx_qid allocation failed for PMD on core %2d" + ", numa_id %d.", pmd->core_id, pmd->numa_id); + OVS_NOT_REACHED(); + } + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); + + VLOG_DBG("static_tx_qid = %d allocated for PMD thread on core %2d" + ", numa_id %d.", pmd->static_tx_qid, pmd->core_id, pmd->numa_id); +} + +static void +pmd_free_static_tx_qid(struct dp_netdev_pmd_thread *pmd) +{ + ovs_mutex_lock(&pmd->dp->tx_qid_pool_mutex); + id_pool_free_id(pmd->dp->tx_qid_pool, pmd->static_tx_qid); + ovs_mutex_unlock(&pmd->dp->tx_qid_pool_mutex); +} + static int pmd_load_queues_and_ports(struct dp_netdev_pmd_thread *pmd, struct polled_queue **ppoll_list) @@ -3586,6 +3645,7 @@ pmd_thread_main(void *f_) dpdk_set_lcore_id(pmd->core_id); poll_cnt = pmd_load_queues_and_ports(pmd, &poll_list); reload: + pmd_alloc_static_tx_qid(pmd); emc_cache_init(&pmd->flow_cache); /* List port/core affinity */ @@ -3634,6 +3694,7 @@ reload: dp_netdev_pmd_reload_done(pmd); emc_cache_uninit(&pmd->flow_cache); + pmd_free_static_tx_qid(pmd); if (!exiting) { goto reload; @@ -3760,8 +3821,6 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, pmd->numa_id = numa_id; pmd->need_reload = false; - *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp->poll_threads); - ovs_refcount_init(&pmd->ref_cnt); latch_init(&pmd->exit_latch); pmd->reload_seq = seq_create(); @@ -3782,6 +3841,7 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, * actual thread created for NON_PMD_CORE_ID. */ if (core_id == NON_PMD_CORE_ID) { emc_cache_init(&pmd->flow_cache); + pmd_alloc_static_tx_qid(pmd); } cmap_insert(&dp->poll_threads, CONST_CAST(struct cmap_node *, &pmd->node), hash_int(core_id, 0)); @@ -3824,6 +3884,7 @@ dp_netdev_del_pmd(struct dp_netdev *dp, struct dp_netdev_pmd_thread *pmd) ovs_mutex_lock(&dp->non_pmd_mutex); emc_cache_uninit(&pmd->flow_cache); pmd_free_cached_ports(pmd); + pmd_free_static_tx_qid(pmd); ovs_mutex_unlock(&dp->non_pmd_mutex); } else { latch_set(&pmd->exit_latch); diff --git a/tests/pmd.at b/tests/pmd.at index 5686bed..43bdb1b 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -38,7 +38,7 @@ dnl dnl Whaits for creation of 'n_threads' or at least 1 thread if $1 not dnl passed. Checking starts from line number 'line' in ovs-vswithd.log . m4_define([CHECK_PMD_THREADS_CREATED], [ - PATTERN="Created [[0-9]]* pmd threads on numa node $2" + PATTERN="There are [[0-9]]* pmd threads on numa node $2" line_st=$3 if [[ -z "$line_st" ]] then
Currently, change of 'pmd-cpu-mask' is very heavy operation. It requires destroying of all the PMD threads and creating them back. After that, all the threads will sleep until ports' redistribution finished. This patch adds ability to not stop the datapath while adjusting number/placement of PMD threads. All not affected threads will forward traffic without any additional latencies. id-pool created for static tx queue ids to keep them sequential in a flexible way. non-PMD thread will always have static_tx_qid = 0 as it was before. Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/dpif-netdev.c | 119 +++++++++++++++++++++++++++++++++++++++++------------- tests/pmd.at | 2 +- 2 files changed, 91 insertions(+), 30 deletions(-)