On 05.08.2015 19:26, Daniele Di Proietto wrote: > > > On 05/08/2015 16:42, "Ilya Maximets" <[email protected]> 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" <[email protected]> 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 <[email protected]> >>>> --- >>>> lib/dpif-netdev.c | 102 >>>> ++++++++++++++++++++++++++---------------------------- >>>> lib/netdev.c | 6 ++++ >>>> lib/netdev.h | 1 + >>>> 3 files changed, 57 insertions(+), 52 deletions(-) > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
