[v5] net/virtio-user: fix packed ring server mode
Checks
Commit Message
This patch fixes the situation where data path 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 descriptors directly from the available ring and
the used ring during reconnection. Therefore, the data path can continue.
But for packed ring, when reconnecting to virtio, vhost cannot get the
status of descriptors via the descriptor ring. By resetting Tx
and Rx queues, the data path 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>
---
v5:
* Fixed two spelling mistakes in the commit log.
* Added notice message when resetting vring.
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 | 42 +++++++++++++++
drivers/net/virtio/virtqueue.c | 71 +++++++++++++++++++++++++
drivers/net/virtio/virtqueue.h | 4 ++
4 files changed, 119 insertions(+), 2 deletions(-)
Comments
On 1/15/20 7:13 AM, Xuan Ding wrote:
> This patch fixes the situation where data path 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 descriptors directly from the available ring and
> the used ring during reconnection. Therefore, the data path can continue.
>
> But for packed ring, when reconnecting to virtio, vhost cannot get the
> status of descriptors via the descriptor ring. By resetting Tx
> and Rx queues, the data path 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>
> ---
>
> v5:
> * Fixed two spelling mistakes in the commit log.
> * Added notice message when resetting vring.
>
> 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 | 42 +++++++++++++++
> drivers/net/virtio/virtqueue.c | 71 +++++++++++++++++++++++++
> drivers/net/virtio/virtqueue.h | 4 ++
> 4 files changed, 119 insertions(+), 2 deletions(-)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 1/15/20 7:13 AM, Xuan Ding wrote:
> This patch fixes the situation where data path 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 descriptors directly from the available ring and
> the used ring during reconnection. Therefore, the data path can continue.
>
> But for packed ring, when reconnecting to virtio, vhost cannot get the
> status of descriptors via the descriptor ring. By resetting Tx
> and Rx queues, the data path 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>
> ---
>
> v5:
> * Fixed two spelling mistakes in the commit log.
> * Added notice message when resetting vring.
>
> 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 | 42 +++++++++++++++
> drivers/net/virtio/virtqueue.c | 71 +++++++++++++++++++++++++
> drivers/net/virtio/virtqueue.h | 4 ++
> 4 files changed, 119 insertions(+), 2 deletions(-)
Applied to dpdk-next-virtio/master
Thanks,
Maxime
On 1/15/2020 11:16 AM, Maxime Coquelin wrote:
>
>
> On 1/15/20 7:13 AM, Xuan Ding wrote:
>> This patch fixes the situation where data path 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 descriptors directly from the available ring and
>> the used ring during reconnection. Therefore, the data path can continue.
>>
>> But for packed ring, when reconnecting to virtio, vhost cannot get the
>> status of descriptors via the descriptor ring. By resetting Tx
>> and Rx queues, the data path 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>
>> ---
>>
>> v5:
>> * Fixed two spelling mistakes in the commit log.
>> * Added notice message when resetting vring.
>>
>> 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 | 42 +++++++++++++++
>> drivers/net/virtio/virtqueue.c | 71 +++++++++++++++++++++++++
>> drivers/net/virtio/virtqueue.h | 4 ++
>> 4 files changed, 119 insertions(+), 2 deletions(-)
>
> Applied to dpdk-next-virtio/master
>
This was causing build error [1] on cross compilation [2], I am fixing it while
merging [3], please double check in next-net.
[1]
.../drivers/net/virtio/virtio_user_ethdev.c:44:2: error: implicit declaration of
function ‘rte_delay_ms’; did you mean ‘rte_realloc’?
[-Werror=implicit-function-declaration]
rte_delay_ms(1);
[2]
CROSS=/opt/aarch64/bin/aarch64-buildroot-linux-gnu- make -j64
[3]
diff --git a/drivers/net/virtio/virtio_user_ethdev.c
b/drivers/net/virtio/virtio_user_ethdev.c
index 9c9d3407f..f3b35d1bd 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -13,6 +13,7 @@
#include <rte_ethdev_vdev.h>
#include <rte_bus_vdev.h>
#include <rte_alarm.h>
+#include <rte_cycles.h>
#include "virtio_ethdev.h"
#include "virtio_logs.h"
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Wednesday, January 15, 2020 11:41 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; 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: [dpdk-stable] [PATCH v5] net/virtio-user: fix packed ring server
> mode
>
> On 1/15/2020 11:16 AM, Maxime Coquelin wrote:
> >
> >
> > On 1/15/20 7:13 AM, Xuan Ding wrote:
> >> This patch fixes the situation where data path 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 descriptors directly from the available
> >> ring and the used ring during reconnection. Therefore, the data path can
> continue.
> >>
> >> But for packed ring, when reconnecting to virtio, vhost cannot get
> >> the status of descriptors via the descriptor ring. By resetting Tx
> >> and Rx queues, the data path 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>
> >> ---
> >>
> >> v5:
> >> * Fixed two spelling mistakes in the commit log.
> >> * Added notice message when resetting vring.
> >>
> >> 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 | 42 +++++++++++++++
> >> drivers/net/virtio/virtqueue.c | 71 +++++++++++++++++++++++++
> >> drivers/net/virtio/virtqueue.h | 4 ++
> >> 4 files changed, 119 insertions(+), 2 deletions(-)
> >
> > Applied to dpdk-next-virtio/master
> >
>
> This was causing build error [1] on cross compilation [2], I am fixing it while
> merging [3], please double check in next-net.
>
> [1]
> .../drivers/net/virtio/virtio_user_ethdev.c:44:2: error: implicit declaration of
> function ‘rte_delay_ms’; did you mean ‘rte_realloc’?
> [-Werror=implicit-function-declaration]
>
> rte_delay_ms(1);
>
>
> [2]
> CROSS=/opt/aarch64/bin/aarch64-buildroot-linux-gnu- make -j64
>
>
> [3]
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 9c9d3407f..f3b35d1bd 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -13,6 +13,7 @@
> #include <rte_ethdev_vdev.h>
> #include <rte_bus_vdev.h>
> #include <rte_alarm.h>
> +#include <rte_cycles.h>
>
> #include "virtio_ethdev.h"
> #include "virtio_logs.h"
After my check, the current call path to rte_cycles.h on the x86 platform is virtio_user_ethdev.c -> virtqueue.h -> rte_mempool.h -> rte_spinlock.h -> rte_cycles.h. I find that only rte_spinlock.h on the x86 platform includes the rte_cycles.h, while this file is not included on other platforms, so your fix is right. I also test the new patch(add #include <rte_cycles.h>) on my local, it works fine.
Thank you very much.
Regards,
Xuan
@@ -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)) {
@@ -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,12 @@ 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))
+ PMD_INIT_LOG(NOTICE, "Packets on the fly will be dropped"
+ " when packed ring reconnecting.");
+ virtio_user_reset_queues_packed(eth_dev);
+
ret = virtio_user_start_device(dev);
if (ret < 0)
return -1;
@@ -141,3 +141,74 @@ virtqueue_rxvq_flush(struct virtqueue *vq)
else
virtqueue_rxvq_flush_split(vq);
}
+
+int
+virtqueue_rxvq_reset_packed(struct virtqueue *vq)
+{
+ int size = vq->vq_nentries;
+ struct vq_desc_extra *dxp;
+ struct virtnet_rx *rxvq;
+ uint16_t desc_idx;
+
+ vq->vq_used_cons_idx = 0;
+ vq->vq_desc_head_idx = 0;
+ vq->vq_avail_idx = 0;
+ vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
+ vq->vq_free_cnt = vq->vq_nentries;
+
+ vq->vq_packed.used_wrap_counter = 1;
+ vq->vq_packed.cached_flags = VRING_PACKED_DESC_F_AVAIL;
+ vq->vq_packed.event_flags_shadow = 0;
+ vq->vq_packed.cached_flags |= VRING_DESC_F_WRITE;
+
+ rxvq = &vq->rxq;
+ memset(rxvq->mz->addr, 0, rxvq->mz->len);
+
+ for (desc_idx = 0; desc_idx < vq->vq_nentries; desc_idx++) {
+ dxp = &vq->vq_descx[desc_idx];
+ if (dxp->cookie != NULL) {
+ rte_pktmbuf_free(dxp->cookie);
+ dxp->cookie = NULL;
+ }
+ }
+
+ vring_desc_init_packed(vq, size);
+
+ return 0;
+}
+
+int
+virtqueue_txvq_reset_packed(struct virtqueue *vq)
+{
+ int size = vq->vq_nentries;
+ struct vq_desc_extra *dxp;
+ struct virtnet_tx *txvq;
+ uint16_t desc_idx;
+
+ vq->vq_used_cons_idx = 0;
+ vq->vq_desc_head_idx = 0;
+ vq->vq_avail_idx = 0;
+ vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
+ vq->vq_free_cnt = vq->vq_nentries;
+
+ vq->vq_packed.used_wrap_counter = 1;
+ vq->vq_packed.cached_flags = VRING_PACKED_DESC_F_AVAIL;
+ vq->vq_packed.event_flags_shadow = 0;
+
+ txvq = &vq->txq;
+ memset(txvq->mz->addr, 0, txvq->mz->len);
+ memset(txvq->virtio_net_hdr_mz->addr, 0,
+ txvq->virtio_net_hdr_mz->len);
+
+ for (desc_idx = 0; desc_idx < vq->vq_nentries; desc_idx++) {
+ dxp = &vq->vq_descx[desc_idx];
+ if (dxp->cookie != NULL) {
+ rte_pktmbuf_free(dxp->cookie);
+ dxp->cookie = NULL;
+ }
+ }
+
+ vring_desc_init_packed(vq, size);
+
+ return 0;
+}
@@ -443,6 +443,10 @@ struct rte_mbuf *virtqueue_detach_unused(struct virtqueue *vq);
/* Flush the elements in the used ring. */
void virtqueue_rxvq_flush(struct virtqueue *vq);
+int virtqueue_rxvq_reset_packed(struct virtqueue *vq);
+
+int virtqueue_txvq_reset_packed(struct virtqueue *vq);
+
static inline int
virtqueue_full(const struct virtqueue *vq)
{