From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6A3B7A04FD; Wed, 15 Jan 2020 07:08:53 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7BF911C043; Wed, 15 Jan 2020 07:08:52 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A9D6E1C042; Wed, 15 Jan 2020 07:08:48 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Jan 2020 22:08:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,321,1574150400"; d="scan'208";a="273550437" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 14 Jan 2020 22:08:47 -0800 Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 14 Jan 2020 22:08:46 -0800 Received: from shsmsx605.ccr.corp.intel.com (10.109.6.215) by SHSMSX601.ccr.corp.intel.com (10.109.6.141) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 15 Jan 2020 14:08:45 +0800 Received: from shsmsx605.ccr.corp.intel.com ([10.109.6.215]) by SHSMSX605.ccr.corp.intel.com ([10.109.6.215]) with mapi id 15.01.1713.004; Wed, 15 Jan 2020 14:08:45 +0800 From: "Ding, Xuan" To: Maxime Coquelin , "Bie, Tiwei" , "Wang, Zhihong" , "Liu, Yong" , "Ye, Xiaolong" CC: "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v4] net/virtio-user: fix packed ring server mode Thread-Index: AQHVuWI+3VmwM1yo+0ScnRX5rLrPCqfp3u8AgAGCNlA= Date: Wed, 15 Jan 2020 06:08:44 +0000 Message-ID: <58c76d4c1a4a4c84a10cb883c1c93a1c@intel.com> References: <20191218022406.86245-1-xuan.ding@intel.com> <20191223072519.18899-1-xuan.ding@intel.com> <14e25591-ed6e-6a1d-c20a-ab967c23b7c8@redhat.com> In-Reply-To: <14e25591-ed6e-6a1d-c20a-ab967c23b7c8@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4] net/virtio-user: fix packed ring server mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Maxime,=20 Replies are inline. > -----Original Message----- > From: Maxime Coquelin > Sent: Tuesday, January 14, 2020 11:04 PM > To: Ding, Xuan ; Bie, Tiwei ; W= ang, > Zhihong ; Liu, Yong ; Ye, > Xiaolong > Cc: dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH v4] net/virtio-user: fix packed ring server mode >=20 >=20 >=20 > 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 >=20 > s/discriptors/descriptors/ >=20 > > 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 >=20 > s/discriptor only through/descriptor via/ >=20 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 > > --- > > > > 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 0; i < dev->data->nb_rx_queues; i++) { > > + rxvq =3D dev->data->rx_queues[i]; > > + virtqueue_rxvq_reset_packed(rxvq->vq); > > + virtio_dev_rx_queue_setup_finish(dev, i); > > + } > > + > > + for (i =3D 0; i < dev->data->nb_tx_queues; i++) { > > + txvq =3D dev->data->tx_queues[i]; > > + virtqueue_txvq_reset_packed(txvq->vq); > > + } > > + > > + hw->started =3D 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 =3D &rte_eth_devices[dev->port_id]; > > + struct virtio_hw *hw =3D eth_dev->data->dev_private; > > > > connectfd =3D accept(dev->listenfd, NULL, NULL); > > if (connectfd < 0) > > @@ -51,6 +87,10 @@ virtio_user_server_reconnect(struct virtio_user_dev > > *dev) > > > > dev->features &=3D dev->device_features; > > > > + /* For packed ring, resetting queues is required in reconnection. */ > > + if (vtpci_packed_queue(hw)) > > + virtio_user_reset_queues_packed(eth_dev); >=20 > I think we should log an error here to state that some packets are likely= to be > dropped. >=20 > Other than that, I don't think we have other choices than doing that righ= t now, > so I'll ack your patch once commit message is fixed and error message is = printed > on vrings reset. >=20 Notice message will be added in the new version. This message will remind u= ser that packets on the fly will be dropped when packed ring reconnecting. Thank you for your comments. Regards, Xuan > Thanks, > Maxime