Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>  		return -EOPNOTSUPP;
>  
> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	if (unlikely(!virtqueue_kick(vq)))
> -		return -EIO;
> +	init_completion(&cmd->completion);
>  
> -	while (!virtqueue_get_buf(vq, &len) &&
> -	       !virtqueue_is_broken(vq))
> -		cpu_relax();
> +again:
> +	spin_lock_irqsave(&admin_vq->lock, flags);
> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> +	if (ret < 0) {
> +		if (ret == -ENOSPC) {
> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
> +			cpu_relax();
> +			goto again;
> +		}
> +		goto unlock_err;
> +	}
> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
> +		goto unlock_err;
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>  
> -	if (virtqueue_is_broken(vq))
> -		return -EIO;
> +	wait_for_completion(&cmd->completion);
>  
>  	return 0;
> +
> +unlock_err:
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
> +	return -EIO;
>  }
>  


The reason we had the virtqueue_is_broken check previously,
is because this way surprise removal works: it happens to set
vq broken flag on all vqs.


So if you are changing that to a completion, I think surprise
removal needs to trigger a callback so the completion
can be signalled.

I think the cvq work also needs this, BTW.




-- 
MST