Re: [V9fs-developer] [PATCH] net/9p: Fix a deadlock case in the virtio transport

[Dominique Martinet <asmadeus@codewreck.org>]

jiangyiwen wrote on Sat, Jul 14, 2018:
> When client has multiple threads that issue io requests all the
> time, and the server has a very good performance, it may cause
> cpu is running in the irq context for a long time because it can
> check virtqueue has buf in the *while* loop.
> 
> So we should keep chan->lock in the whole loop.

Hmm, this is generally bad practice to hold a spin lock for long.
In general, spin locks are meant to protect data, not code.

I'd want some numbers to decide on this one, even if I think this
particular case is safe (e.g. this cannot dead-lock)

> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> ---
>  net/9p/trans_virtio.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 05006cb..9b0f5f2 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -148,20 +148,18 @@ static void req_done(struct virtqueue *vq)
> 
>  	p9_debug(P9_DEBUG_TRANS, ": request done\n");
> 
> +	spin_lock_irqsave(&chan->lock, flags);
>  	while (1) {
> -		spin_lock_irqsave(&chan->lock, flags);
>  		req = virtqueue_get_buf(chan->vq, &len);
> -		if (req == NULL) {
> -			spin_unlock_irqrestore(&chan->lock, flags);
> +		if (req == NULL)
>  			break;
> -		}
>  		chan->ring_bufs_avail = 1;
> -		spin_unlock_irqrestore(&chan->lock, flags);
>  		/* Wakeup if anyone waiting for VirtIO ring space. */
>  		wake_up(chan->vc_wq);

In particular, the wake up here echoes to wait events that will
immediately try to grab the lock, and will needlessly spin on it until
this thread is done.
If we do go this way I'd want setting chan->ring_bufs_avail to be done
just before unlocking and the wakeup to be done just after unlocking out
of the loop iff we processed at least one iteration here.

That should also save you precious cpu cycles while under lock :)

-- 
Dominique Martinet