[PATCH 1/2] [net/9p] Set the condition just before waking up.

[PATCH 1/2] [net/9p] Set the condition just before waking up.

[Venkateswararao Jujjuri (JV)]

Given that the sprious wake-ups are common, we need to move the
condition setting right next to the wake_up().  After setting the condition
to req->status = REQ_STATUS_RCVD, sprious wakeups may cause the
virtqueue back on the free list for someone else to use.
This may result in kernel panic while relasing the pinned pages
in p9_release_req_pages().

Also rearranged the while loop in req_done() for better redability.

Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
---
 net/9p/trans_virtio.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 961e025..cb98af9 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -141,33 +141,33 @@ static void req_done(struct virtqueue *vq)
 
 	P9_DPRINTK(P9_DEBUG_TRANS, ": request done\n");
 
-	do {
+	while (1) {
 		spin_lock_irqsave(&chan->lock, flags);
 		rc = virtqueue_get_buf(chan->vq, &len);
 
-		if (rc != NULL) {
-			chan->ring_bufs_avail = 1;
-			spin_unlock_irqrestore(&chan->lock, flags);
-			/* Wakeup if anyone waiting for VirtIO ring space. */
-			wake_up(chan->vc_wq);
-			P9_DPRINTK(P9_DEBUG_TRANS, ": rc %p\n", rc);
-			P9_DPRINTK(P9_DEBUG_TRANS, ": lookup tag %d\n",
-					rc->tag);
-			req = p9_tag_lookup(chan->client, rc->tag);
-			req->status = REQ_STATUS_RCVD;
-			if (req->tc->private) {
-				struct trans_rpage_info *rp = req->tc->private;
-				/*Release pages */
-				p9_release_req_pages(rp);
-				if (rp->rp_alloc)
-					kfree(rp);
-				req->tc->private = NULL;
-			}
-			p9_client_cb(chan->client, req);
-		} else {
+		if (rc == NULL) {
 			spin_unlock_irqrestore(&chan->lock, flags);
+			break;
 		}
-	} while (rc != NULL);
+
+		chan->ring_bufs_avail = 1;
+		spin_unlock_irqrestore(&chan->lock, flags);
+		/* Wakeup if anyone waiting for VirtIO ring space. */
+		wake_up(chan->vc_wq);
+		P9_DPRINTK(P9_DEBUG_TRANS, ": rc %p\n", rc);
+		P9_DPRINTK(P9_DEBUG_TRANS, ": lookup tag %d\n", rc->tag);
+		req = p9_tag_lookup(chan->client, rc->tag);
+		if (req->tc->private) {
+			struct trans_rpage_info *rp = req->tc->private;
+			/*Release pages */
+			p9_release_req_pages(rp);
+			if (rp->rp_alloc)
+				kfree(rp);
+			req->tc->private = NULL;
+		}
+		req->status = REQ_STATUS_RCVD;
+		p9_client_cb(chan->client, req);
+	}
 }
 
 /**
-- 
1.6.5.2

Re: [PATCH 1/2] [net/9p] Set the condition just before waking up.

[Aneesh Kumar K. V]

On Mon, 14 Mar 2011 15:12:41 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
> Given that the sprious wake-ups are common, we need to move the
> condition setting right next to the wake_up().

I don't understand the above. What do you mean by spurious wakeups are
common ? I guess what happened was that when the work queue got woken up
via p9_client_cb, another request req->status was found to be received
(REQ_STATUS_RCVD) because we set it ahead. So it is not that spurious
wakeups are common, but rather it is a bug we had right ?

>  After setting the condition
> to req->status = REQ_STATUS_RCVD, sprious wakeups may cause the
> virtqueue back on the free list for someone else to use.
> This may result in kernel panic while relasing the pinned pages
> in p9_release_req_pages().

But why are we doing that release pages in the req_done path. ? why not
after we wake up the sleeper in p9_client_rpc ?

-aneesh