| Message ID | c5012098ca6b51dfbdcb190f8c4e3c0bf1c965dc.1655224415.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | 5.19 reverts | expand |
On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote: > This reverts commit 3d200242a6c968af321913b635fc4014b238cba4. > > Buffer selection with nops was used for debugging and benchmarking > but > is useless in real life. Let's revert it before it's released. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > fs/io_uring.c | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index bf556f77d4ab..1b95c6750a81 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = { > [IORING_OP_NOP] = { > .audit_skip = 1, > .iopoll = 1, > - .buffer_select = 1, > }, > [IORING_OP_READV] = { > .needs_file = 1, > @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req, > const struct io_uring_sqe *sqe) > */ > static int io_nop(struct io_kiocb *req, unsigned int issue_flags) > { > - unsigned int cflags; > - void __user *buf; > - > - if (req->flags & REQ_F_BUFFER_SELECT) { > - size_t len = 1; > - > - buf = io_buffer_select(req, &len, issue_flags); > - if (!buf) > - return -ENOBUFS; > - } > - > - cflags = io_put_kbuf(req, issue_flags); > - __io_req_complete(req, issue_flags, 0, cflags); > + __io_req_complete(req, issue_flags, 0, 0); > return 0; > } > The liburing test case I added in "buf-ring: add tests that cycle through the provided buffer ring" relies on this. I don't mind either way if this is kept or that liburing patch is reverted, but it should be consistent. What do you think?
On 6/14/22 12:21 PM, Dylan Yudaken wrote: > On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote: >> This reverts commit 3d200242a6c968af321913b635fc4014b238cba4. >> >> Buffer selection with nops was used for debugging and benchmarking >> but >> is useless in real life. Let's revert it before it's released. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> fs/io_uring.c | 15 +-------------- >> 1 file changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index bf556f77d4ab..1b95c6750a81 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = { >> [IORING_OP_NOP] = { >> .audit_skip = 1, >> .iopoll = 1, >> - .buffer_select = 1, >> }, >> [IORING_OP_READV] = { >> .needs_file = 1, >> @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req, >> const struct io_uring_sqe *sqe) >> */ >> static int io_nop(struct io_kiocb *req, unsigned int issue_flags) >> { >> - unsigned int cflags; >> - void __user *buf; >> - >> - if (req->flags & REQ_F_BUFFER_SELECT) { >> - size_t len = 1; >> - >> - buf = io_buffer_select(req, &len, issue_flags); >> - if (!buf) >> - return -ENOBUFS; >> - } >> - >> - cflags = io_put_kbuf(req, issue_flags); >> - __io_req_complete(req, issue_flags, 0, cflags); >> + __io_req_complete(req, issue_flags, 0, 0); >> return 0; >> } >> > > The liburing test case I added in "buf-ring: add tests that cycle > through the provided buffer ring" relies on this. Good point. > I don't mind either way if this is kept or that liburing patch is > reverted, but it should be consistent. What do you think? It was useful for benchmarking as well, but it'd be a trivial patch to do for targeted testing. I'm fine with killing it, but can also be persuaded not to ;-)
On Tue, 2022-06-14 at 12:26 -0600, Jens Axboe wrote: > On 6/14/22 12:21 PM, Dylan Yudaken wrote: > > On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote: > > > This reverts commit 3d200242a6c968af321913b635fc4014b238cba4. > > > > > > Buffer selection with nops was used for debugging and > > > benchmarking > > > but > > > is useless in real life. Let's revert it before it's released. > > > > > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > > > --- > > > fs/io_uring.c | 15 +-------------- > > > 1 file changed, 1 insertion(+), 14 deletions(-) > > > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > > index bf556f77d4ab..1b95c6750a81 100644 > > > --- a/fs/io_uring.c > > > +++ b/fs/io_uring.c > > > @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] > > > = { > > > [IORING_OP_NOP] = { > > > .audit_skip = 1, > > > .iopoll = 1, > > > - .buffer_select = 1, > > > }, > > > [IORING_OP_READV] = { > > > .needs_file = 1, > > > @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb > > > *req, > > > const struct io_uring_sqe *sqe) > > > */ > > > static int io_nop(struct io_kiocb *req, unsigned int > > > issue_flags) > > > { > > > - unsigned int cflags; > > > - void __user *buf; > > > - > > > - if (req->flags & REQ_F_BUFFER_SELECT) { > > > - size_t len = 1; > > > - > > > - buf = io_buffer_select(req, &len, issue_flags); > > > - if (!buf) > > > - return -ENOBUFS; > > > - } > > > - > > > - cflags = io_put_kbuf(req, issue_flags); > > > - __io_req_complete(req, issue_flags, 0, cflags); > > > + __io_req_complete(req, issue_flags, 0, 0); > > > return 0; > > > } > > > > > > > The liburing test case I added in "buf-ring: add tests that cycle > > through the provided buffer ring" relies on this. > > Good point. > > > I don't mind either way if this is kept or that liburing patch is > > reverted, but it should be consistent. What do you think? > > It was useful for benchmarking as well, but it'd be a trivial patch > to > do for targeted testing. > > I'm fine with killing it, but can also be persuaded not to ;-) > I guess it's better to kill the liburing test which can always be made to work with something other than NOP, than keeping code in the kernel just for a liburing test.. I can send a revert now
On 6/15/22 08:33, Dylan Yudaken wrote: > On Tue, 2022-06-14 at 12:26 -0600, Jens Axboe wrote: >> On 6/14/22 12:21 PM, Dylan Yudaken wrote: >>> On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote: >>>> This reverts commit 3d200242a6c968af321913b635fc4014b238cba4. >>>> >>>> Buffer selection with nops was used for debugging and >>>> benchmarking >>>> but >>>> is useless in real life. Let's revert it before it's released. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> fs/io_uring.c | 15 +-------------- >>>> 1 file changed, 1 insertion(+), 14 deletions(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index bf556f77d4ab..1b95c6750a81 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] >>>> = { >>>> [IORING_OP_NOP] = { >>>> .audit_skip = 1, >>>> .iopoll = 1, >>>> - .buffer_select = 1, >>>> }, >>>> [IORING_OP_READV] = { >>>> .needs_file = 1, >>>> @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb >>>> *req, >>>> const struct io_uring_sqe *sqe) >>>> */ >>>> static int io_nop(struct io_kiocb *req, unsigned int >>>> issue_flags) >>>> { >>>> - unsigned int cflags; >>>> - void __user *buf; >>>> - >>>> - if (req->flags & REQ_F_BUFFER_SELECT) { >>>> - size_t len = 1; >>>> - >>>> - buf = io_buffer_select(req, &len, issue_flags); >>>> - if (!buf) >>>> - return -ENOBUFS; >>>> - } >>>> - >>>> - cflags = io_put_kbuf(req, issue_flags); >>>> - __io_req_complete(req, issue_flags, 0, cflags); >>>> + __io_req_complete(req, issue_flags, 0, 0); >>>> return 0; >>>> } >>>> >>> >>> The liburing test case I added in "buf-ring: add tests that cycle >>> through the provided buffer ring" relies on this. >> >> Good point. >> >>> I don't mind either way if this is kept or that liburing patch is >>> reverted, but it should be consistent. What do you think? >> >> It was useful for benchmarking as well, but it'd be a trivial patch >> to >> do for targeted testing. >> >> I'm fine with killing it, but can also be persuaded not to ;-) >> > > I guess it's better to kill the liburing test which can always be made > to work with something other than NOP, than keeping code in the kernel > just for a liburing test.. Can we do same testing but without nops? Didn't read through the tests, but e.g. read(nr_bytes=1) and check all but the first byte?
diff --git a/fs/io_uring.c b/fs/io_uring.c index bf556f77d4ab..1b95c6750a81 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = { [IORING_OP_NOP] = { .audit_skip = 1, .iopoll = 1, - .buffer_select = 1, }, [IORING_OP_READV] = { .needs_file = 1, @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) */ static int io_nop(struct io_kiocb *req, unsigned int issue_flags) { - unsigned int cflags; - void __user *buf; - - if (req->flags & REQ_F_BUFFER_SELECT) { - size_t len = 1; - - buf = io_buffer_select(req, &len, issue_flags); - if (!buf) - return -ENOBUFS; - } - - cflags = io_put_kbuf(req, issue_flags); - __io_req_complete(req, issue_flags, 0, cflags); + __io_req_complete(req, issue_flags, 0, 0); return 0; }
This reverts commit 3d200242a6c968af321913b635fc4014b238cba4. Buffer selection with nops was used for debugging and benchmarking but is useless in real life. Let's revert it before it's released. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)