[dpdk-dev,v6,1/3] net/virtio: make control queue thread-safe

Message ID 20180107120513.142196-2-xiao.w.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Xiao Wang Jan. 7, 2018, 12:05 p.m. UTC
  The virtio_send_command function may be called from app's configuration
routine, but also from an interrupt handler called when live migration is
done on the backup side. So this patch makes control queue thread-safe
first.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 7 ++++++-
 drivers/net/virtio/virtio_rxtx.c   | 1 +
 drivers/net/virtio/virtio_rxtx.h   | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)
  

Comments

Yuanhan Liu Jan. 8, 2018, 1:06 p.m. UTC | #1
On Sun, Jan 07, 2018 at 04:05:11AM -0800, Xiao Wang wrote:
> diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
> index 54f1e849b..71b5798b0 100644
> --- a/drivers/net/virtio/virtio_rxtx.h
> +++ b/drivers/net/virtio/virtio_rxtx.h
> @@ -84,6 +84,7 @@ struct virtnet_ctl {
>  	rte_iova_t virtio_net_hdr_mem;  /**< hdr for each xmit packet */
>  	uint16_t port_id;               /**< Device port identifier. */
>  	const struct rte_memzone *mz;   /**< mem zone to populate CTL ring. */
> +	rte_spinlock_t sl;              /**< spinlock for control queue. */

It's weird to name it "sl". The typical naming is just "lock".

	--yliu
  
Xiao Wang Jan. 8, 2018, 3:25 p.m. UTC | #2
Hi,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Monday, January 8, 2018 9:07 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: Bie, Tiwei <tiwei.bie@intel.com>; dev@dpdk.org;
> stephen@networkplumber.org
> Subject: Re: [PATCH v6 1/3] net/virtio: make control queue thread-safe
> 
> On Sun, Jan 07, 2018 at 04:05:11AM -0800, Xiao Wang wrote:
> > diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
> > index 54f1e849b..71b5798b0 100644
> > --- a/drivers/net/virtio/virtio_rxtx.h
> > +++ b/drivers/net/virtio/virtio_rxtx.h
> > @@ -84,6 +84,7 @@ struct virtnet_ctl {
> >  	rte_iova_t virtio_net_hdr_mem;  /**< hdr for each xmit packet */
> >  	uint16_t port_id;               /**< Device port identifier. */
> >  	const struct rte_memzone *mz;   /**< mem zone to populate CTL ring.
> */
> > +	rte_spinlock_t sl;              /**< spinlock for control queue. */
> 
> It's weird to name it "sl". The typical naming is just "lock".
> 
> 	--yliu

I'm open to the naming method, but you can see that:
struct rte_mempool_ops_table also has a "sl" field.

BRs,
Xiao
  

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e0328f61d..ac739506e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -177,6 +177,8 @@  virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 		PMD_INIT_LOG(ERR, "Control queue is not supported.");
 		return -1;
 	}
+
+	rte_spinlock_lock(&cvq->sl);
 	vq = cvq->vq;
 	head = vq->vq_desc_head_idx;
 
@@ -184,8 +186,10 @@  virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 		"vq->hw->cvq = %p vq = %p",
 		vq->vq_desc_head_idx, status, vq->hw->cvq, vq);
 
-	if ((vq->vq_free_cnt < ((uint32_t)pkt_num + 2)) || (pkt_num < 1))
+	if (vq->vq_free_cnt < pkt_num + 2 || pkt_num < 1) {
+		rte_spinlock_unlock(&cvq->sl);
 		return -1;
+	}
 
 	memcpy(cvq->virtio_net_hdr_mz->addr, ctrl,
 		sizeof(struct virtio_pmd_ctrl));
@@ -261,6 +265,7 @@  virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 
 	result = cvq->virtio_net_hdr_mz->addr;
 
+	rte_spinlock_unlock(&cvq->sl);
 	return result->status;
 }
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 390c137c8..6a24fdecb 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -407,6 +407,7 @@  virtio_dev_cq_start(struct rte_eth_dev *dev)
 	struct virtio_hw *hw = dev->data->dev_private;
 
 	if (hw->cvq && hw->cvq->vq) {
+		rte_spinlock_init(&hw->cvq->sl);
 		VIRTQUEUE_DUMP((struct virtqueue *)hw->cvq->vq);
 	}
 }
diff --git a/drivers/net/virtio/virtio_rxtx.h b/drivers/net/virtio/virtio_rxtx.h
index 54f1e849b..71b5798b0 100644
--- a/drivers/net/virtio/virtio_rxtx.h
+++ b/drivers/net/virtio/virtio_rxtx.h
@@ -84,6 +84,7 @@  struct virtnet_ctl {
 	rte_iova_t virtio_net_hdr_mem;  /**< hdr for each xmit packet */
 	uint16_t port_id;               /**< Device port identifier. */
 	const struct rte_memzone *mz;   /**< mem zone to populate CTL ring. */
+	rte_spinlock_t sl;              /**< spinlock for control queue. */
 };
 
 int virtio_rxq_vec_setup(struct virtnet_rx *rxvq);