[v2] net/vhost: support queue update

  The commit below changed the readiness condition of vhost device to fix
multi-queues issues showed with QEMU versions.

Now, the vhost device is ready when the first queue-pair is ready.
When more queues are being ready, the queue state callback will be
triggered to notify the vhost manager.

In case of Rx interrupt configuration, the vhost driver set the
kickfd queue file descriptor in order to be notified on Rx traffic.

So, when queue becomes ready, the kickfd may be changed and should be
updated in the Rx interrupt structure.

Update kickfd when the queue state callback is invoked.
Also update event notification when it is enabled by the user.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 49 +++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
  

On 7/21/2020 5:38 PM, Matan Azrad wrote:
> The commit below changed the readiness condition of vhost device to fix
> multi-queues issues showed with QEMU versions.
> 
> Now, the vhost device is ready when the first queue-pair is ready.
> When more queues are being ready, the queue state callback will be
> triggered to notify the vhost manager.
> 
> In case of Rx interrupt configuration, the vhost driver set the
> kickfd queue file descriptor in order to be notified on Rx traffic.
> 
> So, when queue becomes ready, the kickfd may be changed and should be
> updated in the Rx interrupt structure.
> 
> Update kickfd when the queue state callback is invoked.
> Also update event notification when it is enabled by the user.
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Suggested-by: Marvin Liu <yong.liu@intel.com>


This patch addresses a ~%20 performance drop in vhost reported with -rc1.

Although it is missing component maintainers ack, I will merge it relying on
offline discussion Maxime, Matan, Marvin & Chenbo involved, to address the
performance issue observed in -rc2.
Hopefully won't but if we observe issues with this patch, there will be time to
address it before -rc3, I believe it is safer/better to merge it for -rc2 than
holding this fix till -rc3, at least it can be verified this way as part of -rc2.

Applied to dpdk-next-net/master, thanks.
  

Thanks Matan for the fix.

On 7/21/20 6:38 PM, Matan Azrad wrote:
> The commit below changed the readiness condition of vhost device to fix
> multi-queues issues showed with QEMU versions.
> 
> Now, the vhost device is ready when the first queue-pair is ready.
> When more queues are being ready, the queue state callback will be
> triggered to notify the vhost manager.
> 
> In case of Rx interrupt configuration, the vhost driver set the
> kickfd queue file descriptor in order to be notified on Rx traffic.
> 
> So, when queue becomes ready, the kickfd may be changed and should be
> updated in the Rx interrupt structure.
> 
> Update kickfd when the queue state callback is invoked.
> Also update event notification when it is enabled by the user.
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 9fbf39f66b..bbf79b2c0e 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -94,6 +94,7 @@ struct vhost_queue {
>  	struct rte_mempool *mb_pool;
>  	uint16_t port;
>  	uint16_t virtqueue_id;
> +	bool intr_en;
>  	struct vhost_stats stats;
>  };
>  
> @@ -546,6 +547,8 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid)
>  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
>  	rte_wmb();
>  
> +	vq->intr_en = true;
> +
>  	return ret;
>  }
>  
> @@ -571,6 +574,8 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid)
>  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
>  	rte_wmb();
>  
> +	vq->intr_en = false;
> +
>  	return 0;
>  }
>  
> @@ -830,6 +835,45 @@ destroy_device(int vid)
>  	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL);
>  }
>  
> +static int
> +vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
> +{
> +	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
> +	struct pmd_internal *internal = eth_dev->data->dev_private;
> +	struct rte_vhost_vring vring;
> +	struct vhost_queue *vq;
> +	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
> +	int ret = 0;
> +
> +	/*
> +	 * The vring kickfd may be changed after the new device notification.
> +	 * Update it when the vring state is updated.
> +	 */
> +	if (rx_idx >= 0 && rx_idx < eth_dev->data->nb_rx_queues &&
> +	    rte_atomic32_read(&internal->dev_attached) &&
> +	    rte_atomic32_read(&internal->started) &&
> +	    dev_conf->intr_conf.rxq) {

I am not sure we need to filter on rxq, we could just call
rte_vhost_enable_guest_notification(). It would be disabled for tx
queues, as it is done in the .new_device callback so harmless and this
function would be simpler.

But I think your code work, so we should get it in -rc2 to get proper
testing:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

> +		vq = eth_dev->data->rx_queues[rx_idx];
> +		ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
> +		if (!ret) {
> +			if (vring.kickfd !=
> +			    eth_dev->intr_handle->efds[rx_idx]) {
> +				VHOST_LOG(INFO,
> +					  "kickfd for rxq-%d was changed.\n",
> +					  rx_idx);
> +				eth_dev->intr_handle->efds[rx_idx] =
> +								   vring.kickfd;
> +			}
> +
> +			rte_vhost_enable_guest_notification(vid, vring_id,
> +							    vq->intr_en);
> +			rte_wmb();
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int
>  vring_state_changed(int vid, uint16_t vring, int enable)
>  {
> @@ -848,6 +892,11 @@ vring_state_changed(int vid, uint16_t vring, int enable)
>  	eth_dev = list->eth_dev;
>  	/* won't be NULL */
>  	state = vring_states[eth_dev->data->port_id];
> +
> +	if (enable && vring_conf_update(vid, eth_dev, vring))
> +		VHOST_LOG(INFO, "Failed to update vring-%d configuration.\n",
> +			  (int)vring);
> +
>  	rte_spinlock_lock(&state->lock);
>  	if (state->cur[vring] == enable) {
>  		rte_spinlock_unlock(&state->lock);
>