On 2018/7/17 19:42, Dominique Martinet wrote: > >> Subject: net/9p: Fix a deadlock case in the virtio transport > > I hadn't noticed in the v1, but how is that a deadlock fix? > The previous code doesn't look like it deadlocks to me, the commit > message is more correct. >
Hi Dominique, If cpu is running in the irq context for a long time, NMI watchdog will detect the hard lockup in the cpu, and then it will cause kernel panic. So I use this subject to underline the scenario. > jiangyiwen wrote on Tue, Jul 17, 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. >> >> Signed-off-by: Yiwen Jiang <[email protected]> >> --- >> net/9p/trans_virtio.c | 17 ++++++----------- >> 1 file changed, 6 insertions(+), 11 deletions(-) >> >> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c >> index 05006cb..e5fea8b 100644 >> --- a/net/9p/trans_virtio.c >> +++ b/net/9p/trans_virtio.c >> @@ -148,20 +148,15 @@ static void req_done(struct virtqueue *vq) >> >> p9_debug(P9_DEBUG_TRANS, ": request done\n"); >> >> - while (1) { >> - spin_lock_irqsave(&chan->lock, flags); >> - req = virtqueue_get_buf(chan->vq, &len); >> - if (req == NULL) { >> - spin_unlock_irqrestore(&chan->lock, flags); >> - 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); >> + spin_lock_irqsave(&chan->lock, flags); >> + while ((req = virtqueue_get_buf(chan->vq, &len)) != NULL) { >> if (len) >> p9_client_cb(chan->client, req, REQ_STATUS_RCVD); >> } >> + chan->ring_bufs_avail = 1; > > Do we have a guarantee that req_done is only called if there is at least > one buf to read? > For example, that there isn't two threads queueing the same callback but > the first one reads everything and the second has nothing to read? > > If virtblk_done takes care of setting up a "req_done" bool to only > notify waiters if something has been done I'd rather have a reason to do > differently, even if you can argue that nothing bad will happen in case > of a gratuitous wake_up > Sorry, I don't fully understand what your mean. I think even if the ring buffer don't have the data, wakeup operation will not cause any other problem, and the loss of performance can be ignored. Thanks. >> + spin_unlock_irqrestore(&chan->lock, flags); >> + /* Wakeup if anyone waiting for VirtIO ring space. */ >> + wake_up(chan->vc_wq); >> } > > Thanks, >
