[ovs-dev] [PATCH] dpif-netdev: proper tx queue id

Ilya Maximets i.maximets at samsung.com
Wed Aug 5 16:47:36 UTC 2015



On 05.08.2015 19:26, Daniele Di Proietto wrote:
> 
> 
> On 05/08/2015 16:42, "Ilya Maximets" <i.maximets at samsung.com> wrote:
> 
>> Sorry, I agree that example is incorrect. It is really not true, because
>> of using ovs_numa_get_n_cores() to call netdev_set_multiq().
>> No, I didn't actually observe a bug.
>>
>> But there is another example:
>>
>> 	same configuration(2 pmd threads with 1 port,
>> 	2 rxqs per port and pmd-cpu-mask = 00000014).
>>
>> 	pmd_1->tx_qid = 2, pmd_2->tx_qid = 4,
>> 	txq_needs_locking = true (if device hasn't ovs_numa_get_n_cores() queues)
>>
>> 	Lets netdev->real_n_txq = 2; (device has 2 queues)
>>
>> 	In that case, after truncating in netdev_dpdk_send__()
>> 		'qid = qid % dev->real_n_txq;'
>> 	pmd_1: qid = 2 % 2 = 0
>> 	pmd_2: qid = 4 % 2 = 0
>>
>> 	So, both threads will call dpdk_queue_pkts() with same qid = 0.
>> 	This is unexpected behavior if there is 2 tx queues in device.
>> 	Queue #1 will not be used and both threads will lock queue #0
>> 	on each send.
> 
> Yes, that is true.  In general it is hard to properly distribute
> transmission queues because potentially every pmd thread can send
> a packet to any netdev.
> 
> I agree that we can do better than this. Currently we create a txq
> for every core (not for every pmd thread), because we don't want
> to reconfigure every device when a new thread is added (I rembember
> discussing this with Alex, perhaps he can provide more insight).
> We can always change the code to create one txq for every pmd
> thread (I'd appreciate other opinions on this).
> 
>>
>> About your example:
>> 	2 pmd threads can't call netdev_send() with same tx_qid,
>> 	because pmd->tx_qid = pmd->core_id and there is only one thread
>> 	with core_id = 0. See dp_netdev_configure_pmd().
>>
>> 	So,
>> 	pmd1 will call netdev_send(netdev=dpdk0, tx_qid= *pmd1->core_id* )
>> 	pmd2 will call netdev_send(netdev=dpdk0, tx_qid= *pmd2->core_id* )
> 
> I agree, on current master they can't. I meant with this patch applied.
> 

Yes, with patch applied calls with same tx_qid can happen concurrently,
but there is rte_spinlock_lock(&dev->tx_q[qid].tx_lock) for that case
inside netdev_dpdk_send__(). Other netdevs doesn't use qid at all.

>>
>> 	
>> On 05.08.2015 17:54, Daniele Di Proietto wrote:
>>>
>>>
>>> On 05/08/2015 13:28, "Ilya Maximets" <i.maximets at samsung.com> wrote:
>>>
>>>> Currently tx_qid is equal to pmd->core_id. This leads to wrong
>>>> behavior if pmd-cpu-mask different from '/(0*)(1|3|7)?(f*)/',
>>>> e.g. if core_ids are not sequential, or doesn't start from 0, or both.
>>>>
>>>> Example(just one of possible wrong scenarios):
>>>>
>>>> 	starting 2 pmd threads with 1 port, 2 rxqs per port and
>>>> 	pmd-cpu-mask = 00000014.
>>>>
>>>> 	It that case pmd_1->tx_qid = 2, pmd_2->tx_qid = 4 and
>>>> 	txq_needs_locking = false (if device has 2 queues).
>>>>
>>>> 	While netdev_dpdk_send__() qid will not be truncated and
>>>> 	dpdk_queue_pkts() will be called for nonexistent queues (2 and 4).
>>>
>>> This shouldn't be possible, because the datapath requests one txq
>>> for each core in the system, not for each core in pmd-cpu-mask
>>> (see the calls to netdev_set_multiq() in dpif-netdev.c).
>>>
>>> Did you actually observe a bug or an unexpected behaviour?
>>>
>>> I didn't read the patch carefully (I want to understand the problem
>>> first),
>>> but it appears that two pmd threads could call netdev_send on the same
>>> port, with the same tx_qid concurrently.  Example:
>>>
>>>     pmd1 is processing dpdk0 with rx_qid 0, pmd2 is processing dpdk1
>>> with
>>>     rx_qid 0.
>>>
>>>     The flow table is configured to send everything to dpdk0.
>>>
>>>     pmd1 will call netdev_send(netdev=dpdk0, tx_qid=0)
>>>     pmd2 will call netdev_send(netdev=dpdk0, tx_qid=0)
>>>     
>>>     these calls can happen concurrently
>>>
>>>>
>>>> Fix that by calculating tx_qid from rxq indexes for each rxq
>>>> separately.
>>>> 'rxq_poll' structure supplemented by tx_qid and renamed to 'q_poll'.
>>>> 'poll_list' moved inside dp_netdev_pmd_thread structure to be able
>>>> to get proper tx_qid for current port while calling netdev_send().
>>>> Also, information about queues of each thread added to log.
>>>>
>>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>>> ---
>>>> lib/dpif-netdev.c | 102
>>>> ++++++++++++++++++++++++++----------------------------
>>>> lib/netdev.c      |   6 ++++
>>>> lib/netdev.h      |   1 +
>>>> 3 files changed, 57 insertions(+), 52 deletions(-)
> 
> 



More information about the dev mailing list