net/9p: avoid request size exceed to the virtqueue number in the zero copy
Subsystems (3)
PATCH 1/1 net/9p: avoid request size exceed to the virtqueue number in the zero copy
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.
--- net/9p/trans_virtio.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
net/9p/trans_virtio.c +5 -4
@@ -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, };
Signature · 1 line
1.8.3.1
Dominique Martinet
maintainerjiangyiwen wrote on Fri, Aug 03, 2018:
Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 10 lines
Signed-off-by Yiwen Jiang <[email protected]>--- net/9p/trans_virtio.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)net/9p/trans_virtio.c +0
-0
net/9p/trans_virtio.c static void p9_virtio_remove(struct virtio_device *vdev) · line 757
Quoted 7 lines
net/9p/trans_virtio.c +1
-3@@ -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
net/9p/trans_virtio.c static void p9_virtio_remove(struct virtio_device *vdev) · line 765
Quoted 9 lines
net/9p/trans_virtio.c +4
-1+* (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,
Signature · 1 line
Dominique
Quoted 5 lines · Dominique Martinet
jiangyiwen wrote on Fri, Aug 03, 2018:
Quoted 4 lines
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.
Quoted 9 lines
Quoted 1 line
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_IOHDRSZat 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.
Quoted 17 lines
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.
Quoted 2 lines
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...
Quoted 5 lines
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.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Yiwen Jiang <[email protected]>--- net/9p/trans_virtio.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Quoted 15 lines
net/9p/trans_virtio.c static void p9_virtio_remove(struct virtio_device *vdev) · line 757
Quoted 12 lines
net/9p/trans_virtio.c +1
-3@@ -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.
Quoted 13 lines
net/9p/trans_virtio.c static void p9_virtio_remove(struct virtio_device *vdev) · line 765
Quoted 10 lines
net/9p/trans_virtio.c +4
-1+* (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 Martinet
maintainerjiangyiwen wrote on Fri, Aug 03, 2018:
Quoted 15 lines · Dominique Martinet
Quoted 5 lines
jiangyiwen wrote on Fri, Aug 03, 2018:
Quoted 4 lines
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.
Quoted 8 lines
Quoted 1 line
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_IOHDRSZat 24), couldn't we just cheat and not use the start of the buffer if we detect it's overlapping?
Quoted 5 lines
Actually, generally the
P9_IOHDRSZwill not cause the problem, because 24 bytes is too small, butP9_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 useP9_IOHDRSZinstead.
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)
Signature · 1 line
Dominique
Hi Yiwen,
Quoted 16 lines · 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 <[email protected]>--- net/9p/trans_virtio.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)net/9p/trans_virtio.c +0
-0
net/9p/trans_virtio.c static void p9_virtio_remove(struct virtio_device *vdev) · line 760
Quoted 10 lines
net/9p/trans_virtio.c +4
-3@@ -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
Quoted 7 lines
net/9p/trans_virtio.c +1
-1*/ - .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3), + .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5), .def = 1, .owner = THIS_MODULE, };