net/9p: Fix a deadlock case in the virtio transport

PATCH Superseded jiangyiwen 2 participants 7 messages Jul 14, 2018 – Jul 17, 2018 9P file system

Share & tools

jiangyiwen

author

2018-07-14 08:48 UTC #0 lore raw

PATCH 1/1 net/9p: Fix a deadlock case in the virtio transport

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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

net/9p/trans_virtio.c +3 -5

index 05006cb..9b0f5f2 100644
@@ -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);
 		if (len)
 			p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
 	}
+	spin_unlock_irqrestore(&chan->lock, flags);
 }
 
 /**

Signature · 1 line

1.8.3.1

Dominique Martinet

maintainer

2018-07-14 09:05 UTC #1 lore raw

jiangyiwen wrote on Sat, Jul 14, 2018:

Quoted 6 lines

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)

Quoted 16 lines

Signed-off-by Yiwen Jiang <[email protected]>

---
 net/9p/trans_virtio.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

net/9p/trans_virtio.c +1 -1

index 05006cb..9b0f5f2 100644
@@ -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);

net/9p/trans_virtio.c static void req_done(struct virtqueue *vq) · line 158

Quoted 10 lines

net/9p/trans_virtio.c +1 -4

 		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 :)

Signature · 1 line

Dominique Martinet

jiangyiwen

author

2018-07-14 11:18 UTC #2 lore raw

Quoted 6 lines · Dominique Martinet

jiangyiwen wrote on Sat, Jul 14, 2018:
Quoted 5 lines
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.

Quoted 8 lines

Quoted 1 line
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)

Actually, the loop will not hold a spin lock for long, because other threads will not issue new requests in this case. In addition, virtio-blk or virtio-scsi also use this solution, I guess it may also encounter this problem before.

Quoted 25 lines

Signed-off-by Yiwen Jiang <[email protected]>

---
 net/9p/trans_virtio.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

net/9p/trans_virtio.c +2 -5

index 05006cb..9b0f5f2 100644
@@ -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. */

Quoted 9 lines

net/9p/trans_virtio.c static void req_done(struct virtqueue *vq) · line 158
Quoted 1 line
net/9p/trans_virtio.c +0 -0
 		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.

I can move the wakeup operation after the unlocking. Like what I said above, I think this loop will not execute for long.

Thanks,
Yiwen.

Quoted 2 lines

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

Dominique Martinet

maintainer

2018-07-14 12:49 UTC #3 lore raw

jiangyiwen wrote on Sat, Jul 14, 2018:

Quoted 12 lines · Dominique Martinet

Quoted 11 lines
jiangyiwen wrote on Sat, Jul 14, 2018:
Quoted 6 lines
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.

Quoted 8 lines

Quoted 3 lines
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)
Actually, the loop will not hold a spin lock for long, because other threads will not issue new requests in this case. In addition, virtio-blk or virtio-scsi also use this solution, I guess it may also encounter this problem before.

Fair enough. If you do have some numbers to give though (throughput and/or iops before/after) I'd still be really curious.

Quoted 12 lines

Quoted 11 lines
net/9p/trans_virtio.c static void req_done(struct virtqueue *vq) · line 158
Quoted 4 lines
net/9p/trans_virtio.c +0 -1
 		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.

Quoted 2 lines

I can move the wakeup operation after the unlocking. Like what I said above, I think this loop will not execute for long.

Please do, you listed virtio_blk as doing this and they have the same kind of pattern with a req_done bool and only restarting stopped queues if they processed something

Signature · 1 line

Dominique

jiangyiwen

author

2018-07-16 01:58 UTC #4 lore raw

Quoted 37 lines · Dominique Martinet

jiangyiwen wrote on Sat, Jul 14, 2018:
Quoted 12 lines · Dominique Martinet
Quoted 11 lines
jiangyiwen wrote on Sat, Jul 14, 2018:
Quoted 6 lines
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.
Quoted 8 lines
Quoted 3 lines
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)
Actually, the loop will not hold a spin lock for long, because other threads will not issue new requests in this case. In addition, virtio-blk or virtio-scsi also use this solution, I guess it may also encounter this problem before.
Fair enough. If you do have some numbers to give though (throughput and/or iops before/after) I'd still be really curious.
Quoted 12 lines
Quoted 11 lines
net/9p/trans_virtio.c static void req_done(struct virtqueue *vq) · line 158
Quoted 4 lines
net/9p/trans_virtio.c +0 -1
 		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.

Quoted 7 lines

Quoted 2 lines
I can move the wakeup operation after the unlocking. Like what I said above, I think this loop will not execute for long.
Please do, you listed virtio_blk as doing this and they have the same kind of pattern with a req_done bool and only restarting stopped queues if they processed something

You're right, this wake up operation should be put after the unlocking, I will resend it. In addition, whether I should resend this patch based on your 9p-next branch?

Thanks,
Yiwen.

Dominique Martinet

maintainer

2018-07-16 13:39 UTC #5 lore raw

jiangyiwen wrote on Mon, Jul 16, 2018:

Quoted 3 lines

You're right, this wake up operation should be put after the unlocking, I will resend it. In addition, whether I should resend this patch based on your 9p-next branch?

There is a trivial conflict with Thomas' validate PDU length patch, but as it is trivial either work for me - pick whichever is easier to work with for you.

The main reason I asked for a new version of the other patch is that the IDR rework changed spin locks, so I'd rather it being clean.

Thanks,

Signature · 1 line

Dominique

jiangyiwen

author

2018-07-17 01:12 UTC #6 lore raw

Quoted 9 lines · Dominique Martinet

jiangyiwen wrote on Mon, Jul 16, 2018:
Quoted 3 lines
You're right, this wake up operation should be put after the unlocking, I will resend it. In addition, whether I should resend this patch based on your 9p-next branch?
There is a trivial conflict with Thomas' validate PDU length patch, but as it is trivial either work for me - pick whichever is easier to work with for you.

Quoted 6 lines

The main reason I asked for a new version of the other patch is that the IDR rework changed spin locks, so I'd rather it being clean.
Thanks,

ok, I will resend the patch later based on linux-next branch.

Thanks,
Yiwen.