patches for DPDK stable branches
 help / color / Atom feed
From: "Ding, Xuan" <xuan.ding@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
	"Bie, Tiwei" <tiwei.bie@intel.com>,
	"Wang, Zhihong" <zhihong.wang@intel.com>,
	"Liu, Yong" <yong.liu@intel.com>,
	"Ye, Xiaolong" <xiaolong.ye@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH v4] net/virtio-user: fix packed ring server mode
Date: Wed, 15 Jan 2020 06:08:44 +0000
Message-ID: <58c76d4c1a4a4c84a10cb883c1c93a1c@intel.com> (raw)
In-Reply-To: <14e25591-ed6e-6a1d-c20a-ab967c23b7c8@redhat.com>

Hi Maxime, 

Replies are inline.

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, January 14, 2020 11:04 PM
> To: Ding, Xuan <xuan.ding@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>; Liu, Yong <yong.liu@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v4] net/virtio-user: fix packed ring server mode
> 
> 
> 
> On 12/23/19 8:25 AM, Xuan Ding wrote:
> > This patch fixes the situation where datapath does not work properly
> > when vhost reconnects to virtio in server mode with packed ring.
> >
> > Currently, virtio and vhost share memory of vring. For split ring,
> > vhost can read the status of discriptors directly from the available
> > ring and
> 
> s/discriptors/descriptors/
> 
> > the used ring during reconnection. Therefore, the datapath can continue.
> >
> > But for packed ring, when reconnecting to virtio, vhost cannot get the
> > status of discriptors only through the descriptor ring. By resetting
> > Tx
> 
> s/discriptor only through/descriptor via/
> 

Thanks, this will be corrected in the new version.

> > and Rx queues, the datapath can restart from the beginning.
> >
> > Fixes: 4c3f5822eb214 ("net/virtio: add packed virtqueue defines")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > ---
> >
> > v4:
> > * Moved change log below '---' marker.
> >
> > v3:
> > * Removed an extra asterisk from a comment.
> > * Renamed device reset function and moved it to virtio_user_ethdev.c.
> >
> > v2:
> > * Renamed queue reset functions and moved them to virtqueue.c.
> >
> >  drivers/net/virtio/virtio_ethdev.c      |  4 +-
> >  drivers/net/virtio/virtio_user_ethdev.c | 40 ++++++++++++++
> >  drivers/net/virtio/virtqueue.c          | 71 +++++++++++++++++++++++++
> >  drivers/net/virtio/virtqueue.h          |  4 ++
> >  4 files changed, 117 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index 044eb10a7..f9d0ea70d 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1913,6 +1913,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  			goto err_vtpci_init;
> >  	}
> >
> > +	rte_spinlock_init(&hw->state_lock);
> > +
> >  	/* reset device and negotiate default features */
> >  	ret = virtio_init_device(eth_dev,
> VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
> >  	if (ret < 0)
> > @@ -2155,8 +2157,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >  			return -EBUSY;
> >  		}
> >
> > -	rte_spinlock_init(&hw->state_lock);
> > -
> >  	hw->use_simple_rx = 1;
> >
> >  	if (vtpci_with_feature(hw, VIRTIO_F_IN_ORDER)) { diff --git
> > a/drivers/net/virtio/virtio_user_ethdev.c
> > b/drivers/net/virtio/virtio_user_ethdev.c
> > index 3fc172573..425f48230 100644
> > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > @@ -25,12 +25,48 @@
> >  #define virtio_user_get_dev(hw) \
> >  	((struct virtio_user_dev *)(hw)->virtio_user_dev)
> >
> > +static void
> > +virtio_user_reset_queues_packed(struct rte_eth_dev *dev) {
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +	struct virtnet_rx *rxvq;
> > +	struct virtnet_tx *txvq;
> > +	uint16_t i;
> > +
> > +	/* Add lock to avoid queue contention. */
> > +	rte_spinlock_lock(&hw->state_lock);
> > +	hw->started = 0;
> > +
> > +	/*
> > +	 * Waitting for datapath to complete before resetting queues.
> > +	 * 1 ms should be enough for the ongoing Tx/Rx function to finish.
> > +	 */
> > +	rte_delay_ms(1);
> > +
> > +	/* Vring reset for each Tx queue and Rx queue. */
> > +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +		rxvq = dev->data->rx_queues[i];
> > +		virtqueue_rxvq_reset_packed(rxvq->vq);
> > +		virtio_dev_rx_queue_setup_finish(dev, i);
> > +	}
> > +
> > +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +		txvq = dev->data->tx_queues[i];
> > +		virtqueue_txvq_reset_packed(txvq->vq);
> > +	}
> > +
> > +	hw->started = 1;
> > +	rte_spinlock_unlock(&hw->state_lock);
> > +}
> > +
> > +
> >  static int
> >  virtio_user_server_reconnect(struct virtio_user_dev *dev)  {
> >  	int ret;
> >  	int connectfd;
> >  	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
> > +	struct virtio_hw *hw = eth_dev->data->dev_private;
> >
> >  	connectfd = accept(dev->listenfd, NULL, NULL);
> >  	if (connectfd < 0)
> > @@ -51,6 +87,10 @@ virtio_user_server_reconnect(struct virtio_user_dev
> > *dev)
> >
> >  	dev->features &= dev->device_features;
> >
> > +	/* For packed ring, resetting queues is required in reconnection. */
> > +	if (vtpci_packed_queue(hw))
> > +		virtio_user_reset_queues_packed(eth_dev);
> 
> I think we should log an error here to state that some packets are likely to be
> dropped.
> 
> Other than that, I don't think we have other choices than doing that right now,
> so I'll ack your patch once commit message is fixed and error message is printed
> on vrings reset.
> 

Notice message will be added in the new version. This message will remind user that packets on the fly will be dropped when packed ring reconnecting.
Thank you for your comments.

Regards,
Xuan

> Thanks,
> Maxime


  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-09 16:49 [dpdk-stable] [PATCH v1] " Xuan Ding
2019-12-09  8:51 ` [dpdk-stable] [dpdk-dev] " Liu, Yong
2019-12-12 11:08   ` Ding, Xuan
2019-12-18  2:24 ` [dpdk-stable] [PATCH v3] " Xuan Ding
2019-12-18  2:25   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-12-18  2:38     ` Ding, Xuan
2019-12-23  7:25   ` [dpdk-stable] [PATCH v4] " Xuan Ding
2020-01-14 15:04     ` Maxime Coquelin
2020-01-15  6:08       ` Ding, Xuan [this message]
2020-01-15  6:13     ` [dpdk-stable] [PATCH v5] " Xuan Ding
2020-01-15 10:47       ` Maxime Coquelin
2020-01-15 11:16       ` Maxime Coquelin
2020-01-15 15:40         ` Ferruh Yigit
2020-01-16  7:13           ` Ding, Xuan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=58c76d4c1a4a4c84a10cb883c1c93a1c@intel.com \
    --to=xuan.ding@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=tiwei.bie@intel.com \
    --cc=xiaolong.ye@intel.com \
    --cc=yong.liu@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox