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

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

[jiangyiwen]

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.

Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
---
 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
+	 * (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,
 };
-- 
1.8.3.1

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

[Dominique Martinet]

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?

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.



> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> ---
>  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

> +	 * (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,
-- 
Dominique

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@huawei.com>
>> ---
>>  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,
> 

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

[Dominique Martinet]

jiangyiwen wrote on Fri, Aug 03, 2018:
> 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.

The reason is historical (for non-dotl versions of 9P), but the reply if
error could be longer than P9_IOHDRSZ in this case - see the code in
p9_check_zc_errors that copies the end of the string back from the zc
buffer to the 4k buffer

I don't see much other reason, though... But I don't understand how that
is a problem - we don't actually put the full P9_ZC_HDR_SZ in chan->sg
for headers, but these:
        out = pack_sg_list(chan->sg, 0,
                           VIRTQUEUE_NUM, req->tc.sdata, req->tc.size);
        in = pack_sg_list(chan->sg, out,
                          VIRTQUEUE_NUM, req->rc.sdata, in_hdr_len);

req->tc.size is the size of the header up till that point and in_hdr_len
is the size expected from the header.
That's why I suggested that if these are on a page boundary, we could
just memcpy that data further within the P9_ZC_HDR_SZ-sized buffer and
use that for the header (then move it back to the start of the buffer
for the reply header)

-- 
Dominique

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

[piaojun]

Hi Yiwen,

On 2018/8/3 14:50, jiangyiwen wrote:
> 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.
> 
> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> ---
>  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
> +	 * (input + response headers + data pages), address
> +	 * that are not at page boundary, that can result in
> +	 * an extra page in zero copy.

Here should be two extra pages.

Thanks,
Jun

>  	 */
> -	.maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),
> +	.maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5),
>  	.def = 1,
>  	.owner = THIS_MODULE,
>  };
>