[ovs-dev] 答复: [PATCH] netdev-dpdk: shrink critical region under tx_q[qid].tx_lock
Li,Rongqing
lirongqing at baidu.com
Mon Feb 11 08:25:37 UTC 2019
> -----邮件原件-----
> 发件人: Ian Stokes [mailto:ian.stokes at intel.com]
> 发送时间: 2019年2月6日 23:34
> 收件人: Li,Rongqing <lirongqing at baidu.com>; dev at openvswitch.org
> 主题: Re: [ovs-dev] [PATCH] netdev-dpdk: shrink critical region under
> tx_q[qid].tx_lock
>
> On 1/31/2019 2:47 AM, Li RongQing wrote:
> > netdev_dpdk_filter_packet_len() does not need to be protected by
> > tx_q[].tx_lock, and tx_q[].tx_lock can not protect it too, same to
> > netdev_dpdk_qos_run
> >
> > so move them out of this lock to improve the scalability
> >
>
> Thanks for the Patch Li, can you give more details by what you mean in terms of
> scalability? The changes are small beow so I'm curious to as to the usecase you
> have where your seeing an improvement?
>
means to increase parallel
I did not have performance data, but it is strange from code review, critical region
Have some codes which is not need to protect by the same lock
thanks
-RongQing
> > Signed-off-by: Li RongQing <lirongqing at baidu.com>
> > ---
> > lib/netdev-dpdk.c | 33 ++++++++++++++++++++++-----------
> > 1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9e8..bf4918e2c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2333,15 +2333,15 @@ __netdev_dpdk_vhost_send(struct netdev
> *netdev, int qid,
> > goto out;
> > }
> >
> > - rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > -
> > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
> > /* Check has QoS has been configured for the netdev */
> > cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);
> > dropped = total_pkts - cnt;
> >
> > + rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +
> > + int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> > do {
> > - int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> > unsigned int tx_pkts;
> >
> > tx_pkts = rte_vhost_enqueue_burst(vid, vhost_qid, cur_pkts,
> > cnt); @@ -2462,15 +2462,20 @@ netdev_dpdk_send__(struct netdev_dpdk
> *dev, int qid,
> > return;
> > }
> >
> > - if (OVS_UNLIKELY(concurrent_txq)) {
> > - qid = qid % dev->up.n_txq;
> > - rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > - }
> > -
> > if (OVS_UNLIKELY(batch->packets[0]->source != DPBUF_DPDK)) {
> > struct netdev *netdev = &dev->up;
> >
>
> The change below introduces code duplication in both the if and else
> statements specifically for the unlikely case. I'm slow to introduce this change
> as it seems the key benefit is in the case where concurrent txqs are used which
> to date has not been the common use case for the wider community. I take it
> here this case is the beneficiary?
>
> Ian
>
> > + if (OVS_UNLIKELY(concurrent_txq)) {
> > + qid = qid % dev->up.n_txq;
> > + rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > + }
> > +
> > dpdk_do_tx_copy(netdev, qid, batch);
> > +
> > + if (OVS_UNLIKELY(concurrent_txq)) {
> > + rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > + }
> > +
> > dp_packet_delete_batch(batch, true);
> > } else {
> > int tx_cnt, dropped;
> > @@ -2481,8 +2486,17 @@ netdev_dpdk_send__(struct netdev_dpdk *dev,
> int qid,
> > tx_cnt = netdev_dpdk_qos_run(dev, pkts, tx_cnt, true);
> > dropped = batch_cnt - tx_cnt;
> >
> > + if (OVS_UNLIKELY(concurrent_txq)) {
> > + qid = qid % dev->up.n_txq;
> > + rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > + }
> > +
> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, tx_cnt);
> >
> > + if (OVS_UNLIKELY(concurrent_txq)) {
> > + rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > + }
> > +
> > if (OVS_UNLIKELY(dropped)) {
> > rte_spinlock_lock(&dev->stats_lock);
> > dev->stats.tx_dropped += dropped;
> > @@ -2490,9 +2504,6 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int
> qid,
> > }
> > }
> >
> > - if (OVS_UNLIKELY(concurrent_txq)) {
> > - rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> > - }
> > }
> >
> > static int
> >
More information about the dev
mailing list