Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.

["Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>]

On 10/5/2010 7:45 AM, Aneesh Kumar K. V wrote:
> On Fri,  1 Oct 2010 16:56:17 -0700, "Venkateswararao Jujjuri (JV)"<jvrao@linux.vnet.ibm.com>  wrote:
>> If there is not enough space for the PDU on the VirtIO ring, current
>> code returns -EIO propagating the error to user.
>>
>> This patch introduced a wqit_queue on the channel, and lets the process
>> wait on this queue until VirtIO ring frees up.
>>
>> Signed-off-by: Venkateswararao Jujjuri<jvrao@linux.vnet.ibm.com>
>> ---
>>   net/9p/trans_virtio.c |   44 +++++++++++++++++++++++++++++++++++++-------
>>   1 files changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index 0df84bf..2de5144 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -75,6 +75,8 @@ struct virtio_chan {
>>   	struct p9_client *client;
>>   	struct virtio_device *vdev;
>>   	struct virtqueue *vq;
>> +	int ring_bufs_avail;
>> +	wait_queue_head_t *vc_wq;
>>
>>   	/* Scatterlist: can be too big for stack. */
>>   	struct scatterlist sg[VIRTQUEUE_NUM];
>> @@ -141,15 +143,21 @@ static void req_done(struct virtqueue *vq)
>>   	do {
>>   		spin_lock_irqsave(&chan->lock, flags);
>>   		rc = virtqueue_get_buf(chan->vq,&len);
>> -		spin_unlock_irqrestore(&chan->lock, flags);
>>
>>   		if (rc != NULL) {
>> +			if (!chan->ring_bufs_avail) {
>> +				chan->ring_bufs_avail = 1;
>> +				wake_up(chan->vc_wq);
>> +			}
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>>   			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;
>>   			p9_client_cb(chan->client, req);
>> +		} else {
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>>   		}
>>   	} while (rc != NULL);
>>   }
>> @@ -212,6 +220,7 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
>>
>>   	P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request\n");
>>
>> +req_retry:
>>   	req->status = REQ_STATUS_SENT;
>>
>>   	spin_lock_irqsave(&chan->lock, flags);
>> @@ -222,10 +231,21 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
>>
>>   	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc);
>>   	if (err<  0) {
>> -		spin_unlock_irqrestore(&chan->lock, flags);
>> -		P9_DPRINTK(P9_DEBUG_TRANS,
>> -			"9p debug: virtio rpc add_buf returned failure");
>> -		return -EIO;
>> +		if (err == -ENOSPC) {
>> +			chan->ring_bufs_avail = 0;
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +			err = wait_event_interruptible_timeout(*chan->vc_wq,
>> +							chan->ring_bufs_avail,
>> +							HZ/4);
>
> Why do we need this to be _timeout ? Also if interrupted by a signal we do
> want to return to user space with -EIO right ? Or we loop here always ?

We are waiting for some other request to complete and the wait is outside the lock.
Hence it is possible miss the wakwup...hence timed wait.

No We loop here for ever as long as we get -ENOSPC is returned by virtqueue_add_buf.

- JV

>
>> +			P9_DPRINTK(P9_DEBUG_TRANS, "9p:Retry virtio request\n");
>> +			goto req_retry;
>> +		} else {
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +			P9_DPRINTK(P9_DEBUG_TRANS,
>> +					"9p debug: "
>> +					"virtio rpc add_buf returned failure");
>> +			return -EIO;
>> +		}
>>   	}
>
> -aneesh