On Wed, Jun 17, 2015 at 12:47:28PM -0300, Flavio Leitner wrote:
> On Wed, Jun 17, 2015 at 12:28:23PM -0300, Flavio Leitner wrote:
> > On Wed, Jun 17, 2015 at 10:54:25AM +0800, Dongjun wrote:
> > >
> > >
> > > On 2015/6/17 1:44, Daniele Di Proietto wrote:
> > > >
> > > >On 16/06/2015 07:40, "Pravin Shelar" <pshe...@nicira.com> wrote:
> > > >
> > > >>On Mon, Jun 8, 2015 at 7:42 PM, Pravin Shelar <pshe...@nicira.com>
> > > >>wrote:
> > > >>>On Mon, Jun 8, 2015 at 6:13 PM, Wei li <l...@dtdream.com> wrote:
> > > >>>>When tx queue is shared among CPUS,the pkts always be flush in
> > > >>>>'netdev_dpdk_eth_send'
> > > >>>>So it is unnecessarily for flushing in netdev_dpdk_rxq_recv
> > > >>>>Otherwise tx will be accessed without locking
> > > >>>>
> > > >>>>Signed-off-by: Wei li <l...@dtdream.com>
> > > >>>>---
> > > >>>>v1->v2: fix typo
> > > >>>>
> > > >>>> lib/netdev-dpdk.c | 7 +++++--
> > > >>>> 1 file changed, 5 insertions(+), 2 deletions(-)
> > > >>>>
> > > >>>>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > >>>>index 63243d8..1e0a483 100644
> > > >>>>--- a/lib/netdev-dpdk.c
> > > >>>>+++ b/lib/netdev-dpdk.c
> > > >>>>@@ -892,8 +892,11 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_,
> > > >>>>struct dp_packet **packets,
> > > >>>> int nb_rx;
> > > >>>>
> > > >>>> /* There is only one tx queue for this core. Do not flush other
> > > >>>>- * queueus. */
> > > >>>>- if (rxq_->queue_id == rte_lcore_id()) {
> > > >>>>+ * queues.
> > > >>>>+ * Do not flush tx queue which is shared among CPUs
> > > >>>>+ * since it is always flushed */
> > > >>>>+ if (rxq_->queue_id == rte_lcore_id() &&
> > > >>>>+ OVS_LIKELY(!dev->txq_needs_locking)) {
> > > >>>> dpdk_queue_flush(dev, rxq_->queue_id);
> > > >>>> }
> > > >>>Patch looks good, But Daniele has plan to get rid of queue flushing
> > > >>>logic. do you see problem with doing this?
> > > >>Daniele,
> > > >>I am not sure if we should fix this queue flushing logic if we are
> > > >>going to remove it. So I would like to discuss it first.
> > > >>You mentioned that the removing flushing logic results in performance
> > > >>drop. Can you explain it? How much is performance drop and which is
> > > >>the case?
> > > >Hi Pravin,
> > > >
> > > >sorry for the late reply. I suspected that removing the queues in
> > > >netdev-dpdk
> > > >was going to have a performance impact in the following scenario: a batch
> > > >of
> > > >packets from different megaflows with the same action (output on port 1).
> > > >Here's what happens:
> > > >
> > > >1) With the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
> > > >each
> > > > packet. netdev_dpdk_send() just copies the pointer in the queue.
> > > > Before
> > > > receiving the next batch dpdk_queue_flush() call rte_eth_tx_burst()
> > > > with
> > > > a whole batch of packets
> > > >2) Without the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
> > > >each
> > > > packet. netdev_dpdk_send() calls rte_eth_tx_burst() for each packet.
> > > >
> > > >Scenario 2 could be slower because rte_eth_tx_burst() is called for each
> > > >packet
> > > >(instead of each batch). After some testing this turns out not to be the
> > > >case.
> > > >It appears that the bottleneck is not rte_eth_tx_burst(), and copying the
> > > >pointers
> > > >in the temporary queue makes the code slower.
> > > Hi Daniele,
> > > I get something else impacting the performance too. Here it is:
> > > In netdev_dpdk_rxq_recv(), the flush condition is "txq->count != 0", and
> > > in
> > > dpdk_queue_pkts(), the flush condition is "txq->count == MAX_TX_QUEUE_LEN"
> > > or "(rte_get_timer_cycles() - txq->tsc) >= DRAIN_TSC".
> > > They don't match. Our thoughts is to get a batch flush, but
> > > netdev_dpdk_rxq_recv() may do it earlier sometime.
> > >
> > > I did some local change, stealing the cycle interval from
> > > dpdk_queue_pkts().
> > > I got performance improvement, about at least 10% from guest VNIC to dpdk
> > > phy NIC to outside.
> > > I didn't compare the performance to "Scenario 2" for "Scenario 2"
> > > modification is not that easy. :(
> > >
> > > $ git diff
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > index 3af1ee7..98d33c3 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -929,10 +929,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_, struct
> > > dp_packet **packets,
> > >
> > > /* There is only one tx queue for this core. Do not flush other
> > > * queueus. */
> > > - if (rxq_->queue_id == rte_lcore_id()) {
> > > - dpdk_queue_flush(dev, rxq_->queue_id);
> > > - }
> > > + if (rxq_->queue_id == rte_lcore_id() &&
> > > OVS_LIKELY(!dev->txq_needs_locking)) {
> > > + struct dpdk_tx_queue *txq = &dev->tx_q[rxq_->queue_id];
> > >
> > > + if (txq->count != 0 && (rte_get_timer_cycles() - txq->tsc) >=
> > > DRAIN_TSC) {
> > > + dpdk_queue_flush__(dev, rxq_->queue_id); // instead of
> > > dpdk_queue_flush
> > > + }
> > > + }
> > > nb_rx = rte_eth_rx_burst(rx->port_id, rxq_->queue_id,
> > > (struct rte_mbuf **) packets,
> > > NETDEV_MAX_BURST);
Before patch: 3.5Mpps
After patch: 3.3Mpps
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
