I agree. It made sense with my first fix. Fixed and sent again.
Thanks.
On 27.07.2015 19:58, Daniele Di Proietto wrote:
> Thanks for the updated patch.
>
> One comment below
>
> On 27/07/2015 13:19, "Ilya Maximets" <i.maxim...@samsung.com> wrote:
>
>> Currently pmd threads select queues in pmd_load_queues() according to
>> get_n_pmd_threads_on_numa(). This behavior leads to race between pmds,
>> beacause dp_netdev_set_pmds_on_numa() starts them one by one and
>> current number of threads changes incrementally.
>>
>> As a result we may have the following situation with 2 pmd threads:
>>
>> * dp_netdev_set_pmds_on_numa()
>> * pmd12 thread started. Currently only 1 pmd thread exists.
>> dpif_netdev(pmd12)|INFO|Core 1 processing port 'port_1'
>> dpif_netdev(pmd12)|INFO|Core 1 processing port 'port_2'
>> * pmd14 thread started. 2 pmd threads exists.
>> dpif_netdev|INFO|Created 2 pmd threads on numa node 0
>> dpif_netdev(pmd14)|INFO|Core 2 processing port 'port_2'
>>
>> We have:
>> core 1 --> port 1, port 2
>> core 2 --> port 2
>>
>> Fix this by starting pmd threads only after all of them have
>> been configured.
>>
>> Cc: Daniele Di Proietto <diproiet...@vmware.com>
>> Cc: Dyasly Sergey <s.dya...@samsung.com>
>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>> ---
>> lib/dpif-netdev.c | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 79c4612..4fca7b7 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -1123,10 +1123,9 @@ do_add_port(struct dp_netdev *dp, const char
>> *devname, const char *type,
>> ovs_refcount_init(&port->ref_cnt);
>> cmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
>>
>> - if (netdev_is_pmd(netdev)) {
>> + if (netdev_is_pmd(netdev))
>> dp_netdev_set_pmds_on_numa(dp, netdev_get_numa_id(netdev));
>> - dp_netdev_reload_pmds(dp);
>> - }
>> +
>
> I think we should still call 'dp_netdev_reload_pmds()' when adding a
> new port.
>
>> seq_change(dp->port_seq);
>>
>> return 0;
>> @@ -2961,6 +2960,7 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp,
>> int numa_id)
>> * pmd threads for the numa node. */
>> if (!n_pmds) {
>> int can_have, n_unpinned, i;
>> + struct dp_netdev_pmd_thread **pmds;
>>
>> n_unpinned = ovs_numa_get_n_unpinned_cores_on_numa(numa_id);
>> if (!n_unpinned) {
>> @@ -2972,15 +2972,22 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp,
>> int numa_id)
>> /* If cpu mask is specified, uses all unpinned cores, otherwise
>> * tries creating NR_PMD_THREADS pmd threads. */
>> can_have = dp->pmd_cmask ? n_unpinned : MIN(n_unpinned,
>> NR_PMD_THREADS);
>> + pmds = xzalloc(can_have * sizeof *pmds);
>> for (i = 0; i < can_have; i++) {
>> - struct dp_netdev_pmd_thread *pmd = xzalloc(sizeof *pmd);
>> unsigned core_id =
>> ovs_numa_get_unpinned_core_on_numa(numa_id);
>> -
>> - dp_netdev_configure_pmd(pmd, dp, i, core_id, numa_id);
>> + pmds[i] = xzalloc(sizeof **pmds);
>> + dp_netdev_configure_pmd(pmds[i], dp, i, core_id, numa_id);
>> + }
>> + /* The pmd thread code needs to see all the others configured pmd
>> + * threads on the same numa node. That's why we call
>> + * 'dp_netdev_configure_pmd()' on all the threads and then we
>> actually
>> + * start them. */
>> + for (i = 0; i < can_have; i++) {
>> /* Each thread will distribute all devices rx-queues among
>> * themselves. */
>> - pmd->thread = ovs_thread_create("pmd", pmd_thread_main, pmd);
>> + pmds[i]->thread = ovs_thread_create("pmd", pmd_thread_main,
>> pmds[i]);
>> }
>> + free(pmds);
>> VLOG_INFO("Created %d pmd threads on numa node %d", can_have,
>> numa_id);
>> }
>> }
>> --
>> 2.1.4
>>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
