Re: [dpdk-dev] [PATCH 38/40] net/virtio: move Vhost-user specifics to its backend

["Xia, Chenbo" <chenbo.xia@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, December 21, 2020 5:14 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
> amorenoz@redhat.com; david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 38/40] net/virtio: move Vhost-user specifics to its backend
> 
> This patch moves all the Vhost-user backend specific
> logic like Vhost FD, listen FD and interrupt handling
> to the vhost-user backend implementation.
> 
> In order to achieve that, new ops are created to update
> the link status, disconnect and reconnect the server,
> and fetch the link state interrupt FD.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost.h        |   4 +
>  drivers/net/virtio/virtio_user/vhost_kernel.c |  18 +-
>  drivers/net/virtio/virtio_user/vhost_user.c   | 169 ++++++++++++++---
>  drivers/net/virtio/virtio_user/vhost_vdpa.c   |  16 ++
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 175 ++++++++++++++---
>  .../net/virtio/virtio_user/virtio_user_dev.h  |   9 +-
>  drivers/net/virtio/virtio_user_ethdev.c       | 179 +-----------------
>  7 files changed, 340 insertions(+), 230 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index ee5598226d..6001d7a164 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -75,6 +75,10 @@ struct virtio_user_backend_ops {
>  	int (*enable_qp)(struct virtio_user_dev *dev, uint16_t pair_idx, int
> enable);
>  	int (*dma_map)(struct virtio_user_dev *dev, void *addr, uint64_t iova,
> size_t len);
>  	int (*dma_unmap)(struct virtio_user_dev *dev, void *addr, uint64_t iova,
> size_t len);
> +	int (*update_link_state)(struct virtio_user_dev *dev);
> +	int (*server_disconnect)(struct virtio_user_dev *dev);
> +	int (*server_reconnect)(struct virtio_user_dev *dev);
> +	int (*get_intr_fd)(struct virtio_user_dev *dev);

This get_intr_fd is for getting link state interrupt fd. I think his name is a little bit
generic. What do you think? Maybe we could change the name or add comment here to clarify here?

>  };
> 
>  extern struct virtio_user_backend_ops virtio_ops_user;
> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
> b/drivers/net/virtio/virtio_user/vhost_kernel.c
> index 023fddcd69..0ba37b23dc 100644
> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
> @@ -459,6 +459,20 @@ vhost_kernel_get_backend_features(uint64_t *features)
>  	return 0;
>  }
> 

<snip>

> +static void
> +virtio_user_dev_reset_queues_packed(struct rte_eth_dev *eth_dev)
> +{
> +	struct virtio_user_dev *dev = eth_dev->data->dev_private;
> +	struct virtio_hw *hw = &dev->hw;
> +	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.

s/Waitting/Waiting

> +	 * 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 < eth_dev->data->nb_rx_queues; i++) {
> +		rxvq = eth_dev->data->rx_queues[i];
> +		virtqueue_rxvq_reset_packed(rxvq->vq);
> +		virtio_dev_rx_queue_setup_finish(eth_dev, i);
> +	}
> +
> +	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +		txvq = eth_dev->data->tx_queues[i];
> +		virtqueue_txvq_reset_packed(txvq->vq);
> +	}
> +
> +	hw->started = 1;
> +	rte_spinlock_unlock(&hw->state_lock);
> +}
> +
> +void
> +virtio_user_dev_delayed_handler(void *param)
> +{
> +	struct virtio_user_dev *dev = param;
> +	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
> +
> +	if (rte_intr_disable(eth_dev->intr_handle) < 0) {
> +		PMD_DRV_LOG(ERR, "interrupt disable failed");
> +		return;
> +	}
> +	rte_intr_callback_unregister(eth_dev->intr_handle,
> +				     virtio_interrupt_handler, eth_dev);
> +	if (dev->is_server) {
> +		dev->ops->server_disconnect(dev);

Better to check ops existence too?

> +		eth_dev->intr_handle->fd = dev->ops->get_intr_fd(dev);
> +		rte_intr_callback_register(eth_dev->intr_handle,
> +					   virtio_interrupt_handler, eth_dev);
> +		if (rte_intr_enable(eth_dev->intr_handle) < 0) {
> +			PMD_DRV_LOG(ERR, "interrupt enable failed");
> +			return;
> +		}
> +	}
> +}
> +
> +int
> +virtio_user_dev_server_reconnect(struct virtio_user_dev *dev)
> +{
> +	int ret, old_status;
> +	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
> +	struct virtio_hw *hw = &dev->hw;
> +
> +	if (!dev->ops->server_reconnect) {
> +		PMD_DRV_LOG(ERR, "(%s) Missing server reconnect callback", dev-
> >path);
> +		return -1;
> +	}

Forget to call server_reconnect? :) 

> +
> +	old_status = virtio_get_status(hw);
> +
> +	virtio_reset(hw);
> +
> +	virtio_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> +
> +	virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> +
> +	if (dev->ops->get_features(dev, &dev->device_features) < 0) {
> +		PMD_INIT_LOG(ERR, "get_features failed: %s",
> +			     strerror(errno));
> +		return -1;
> +	}
> +
> +	dev->device_features |= dev->frontend_features;
> +
> +	/* umask vhost-user unsupported features */

s/umask/unmask

Thanks,
Chenbo