Re: [V9fs-developer] [PATCH] net/9p: avoid request size exceed to the virtqueue number in the zero copy

[jiangyiwen]

On 2018/8/3 15:32, Dominique Martinet wrote:
> jiangyiwen wrote on Fri, Aug 03, 2018:
>> Unfortunately, when the address(input and response headers) are not
>> at page boundary, it will need two extra entry in the zero copy, or
>> else it will cause sg array out of bounds.
>>
>> To avoid the problem, we should subtract two pages for maxsize.
> 
> Good catch, that must have been painful to figure.
> 
> Given we know how big the headers are (something like 11 or 23 bytes
> depending on the op/direction, it's capped by P9_IOHDRSZ at 24),
> couldn't we just cheat and not use the start of the buffer if we detect
> it's overlapping?
> 

Actually, generally the P9_IOHDRSZ will not cause the problem, because
24 bytes is too small, but P9_ZC_HDR_SZ(4096 bytes) often cause two pages.

So I have a question about why we need to use P9_ZC_HDR_SZ, actually we
may use P9_IOHDRSZ instead.

> It's probably faster to memcpy a few bytes than to use two pages for the
> sg list...
> It's definitely ugly though, just taking more margin here is probably
> just as good.
> 
> 
> 
> I'm going to be picky about English again, sorry, please bear with me.
> 
>> Subject: net/9p: avoid request size exceed to the virtqueue number in
>> the zero copy
> 
> This is >72 characters so a little bit too long, if possible to shorten
> it.
> I'm also not sure 'exceed' is a noun so I probably wouldn't have
> understood this sentence without the rest of the message...
> 
> The balance is difficult but it doesn't need to contain too much details
> either something like "9p/virtio: reduce transport maxsize" is simple
> but probably enough as it describes what is done: someone can look at
> the rest of the message for the justification.
> 

Thanks, I will resend the patch later.

> 
> 
>> Signed-off-by: Yiwen Jiang <jiangyiwen@xxxxxxxxxx>
>> ---
>>  net/9p/trans_virtio.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index 6265d1d..63591b2 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -754,11 +754,12 @@ static void p9_virtio_remove(struct virtio_device *vdev)
>>  	.cancel = p9_virtio_cancel,
>>  	/*
>>  	 * We leave one entry for input and one entry for response
>> -	 * headers. We also skip one more entry to accomodate, address
>> -	 * that are not at page boundary, that can result in an extra
>> -	 * page in zero copy.
>> +	 * headers. We also skip three more entrys to accomodate
> 
> "entry"'s plural is "entries", this word is in checkpatch's dictionary
> as a common typo

Thanks, I will modify it.

> 
>> +	 * (input + response headers + data pages), address
>> +	 * that are not at page boundary, that can result in
>> +	 * an extra page in zero copy.
>>  	 */
>> -	.maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),
>> +	.maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5),
>>  	.def = 1,
>>  	.owner = THIS_MODULE,
>>  };
> 
> Thanks,
>