| Message ID | 20230314125727.1731233-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring/ublk: add IORING_OP_FUSED_CMD | expand |
hi, > Hello, > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > and its ->issue() can retrieve/import buffer from master request's > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > submits slave OP just like normal OP issued from userspace, that said, > SQE order is kept, and batching handling is done too. Thanks for this great work, seems that we're now in the right direction to support ublk zero copy, I believe this feature will improve io throughput greatly and reduce ublk's cpu resource usage. I have gone through your 2th patch, and have some little concerns here: Say we have one ublk loop target device, but it has 4 backend files, every file will carry 25% of device capacity and it's implemented in stripped way, then for every io request, current implementation will need issed 4 fused_cmd, right? 4 slave sqes are necessary, but it would be better to have just one master sqe, so I wonder whether we can have another method. The key point is to let io_uring support register various kernel memory objects, which come from kernel, such as ITER_BVEC or ITER_KVEC. so how about below actions: 1. add a new infrastructure in io_uring, which will support to register various kernel memory objects in it, this new infrastructure could be maintained in a xarray structure, every memory objects in it will have a unique id. This registration could be done in a ublk uring cmd, io_uring offers registration interface. 2. then any sqe can use these memory objects freely, so long as it passes above unique id in sqe properly. Above are just rough ideas, just for your reference. And current zero-copy method only supports raw data redirection, if ublk targets need to crc, compress, encrypt raw io requests' pages, then we'll still need to copy block layer's io data to userspace daemon. In that way, ebpf may give a help :) we directly operate block layer's io data in ebpf prog, doing crc or compress, encrypt, still does not need to copy to userspace daemon. But as you said before, ebpf may not support complicated user io logic, a much long way to go... Regards, Xiaoguang Wang > > Please see detailed design in commit log of the 2th patch, and one big > point is how to handle buffer ownership. > > With this way, it is easy to support zero copy for ublk/fuse device. > > Basically userspace can specify any sub-buffer of the ublk block request > buffer from the fused command just by setting 'offset/len' > in the slave SQE for running slave OP. This way is flexible to implement > io mapping: mirror, stripped, ... > > The 3th & 4th patches enable fused slave support for the following OPs: > > OP_READ/OP_WRITE > OP_SEND/OP_RECV/OP_SEND_ZC > > The other ublk patches cleans ublk driver and implement fused command > for supporting zero copy. > > Follows userspace code: > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > > ublk add -t [loop|nbd|qcow2] -z .... > > Basic fs mount/kernel building and builtin test are done, and also not > observe regression on xfstest test over ublk-loop with zero copy. > > Also add liburing test case for covering fused command based on miniublk > of blktest: > > https://github.com/ming1/liburing/commits/fused_cmd_miniublk > > Performance improvement is obvious on memory bandwidth > related workloads, such as, 1~2X improvement on 64K/512K BS > IO test on loop with ramfs backing file. > > Any comments are welcome! > > V3: > - fix build warning reported by kernel test robot > - drop patch for checking fused flags on existed drivers with > ->uring_command(), which isn't necessary, since we do not do that > when adding new ioctl or uring command > - inline io_init_rq() for core code, so just export io_init_slave_req > - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > will be cleared > - pass xfstest over ublk-loop > > V2: > - don't resue io_mapped_ubuf (io_uring) > - remove REQ_F_FUSED_MASTER_BIT (io_uring) > - fix compile warning (io_uring) > - rebase on v6.3-rc1 (io_uring) > - grabbing io request reference when handling fused command > - simplify ublk_copy_user_pages() by iov iterator > - add read()/write() for userspace to read/write ublk io buffer, so > that some corner cases(read zero, passthrough request(report zones)) can > be handled easily in case of zero copy; this way also helps to switch to > zero copy completely > - misc cleanup > > > Ming Lei (16): > io_uring: increase io_kiocb->flags into 64bit > io_uring: add IORING_OP_FUSED_CMD > io_uring: support OP_READ/OP_WRITE for fused slave request > io_uring: support OP_SEND_ZC/OP_RECV for fused slave request > block: ublk_drv: mark device as LIVE before adding disk > block: ublk_drv: add common exit handling > block: ublk_drv: don't consider flush request in map/unmap io > block: ublk_drv: add two helpers to clean up map/unmap request > block: ublk_drv: clean up several helpers > block: ublk_drv: cleanup 'struct ublk_map_data' > block: ublk_drv: cleanup ublk_copy_user_pages > block: ublk_drv: grab request reference when the request is handled by > userspace > block: ublk_drv: support to copy any part of request pages > block: ublk_drv: add read()/write() support for ublk char device > block: ublk_drv: don't check buffer in case of zero copy > block: ublk_drv: apply io_uring FUSED_CMD for supporting zero copy > > drivers/block/ublk_drv.c | 602 ++++++++++++++++++++++++++------- > include/linux/io_uring.h | 49 ++- > include/linux/io_uring_types.h | 80 +++-- > include/uapi/linux/io_uring.h | 1 + > include/uapi/linux/ublk_cmd.h | 37 +- > io_uring/Makefile | 2 +- > io_uring/fused_cmd.c | 245 ++++++++++++++ > io_uring/fused_cmd.h | 11 + > io_uring/io_uring.c | 28 +- > io_uring/io_uring.h | 3 + > io_uring/net.c | 30 +- > io_uring/opdef.c | 17 + > io_uring/opdef.h | 2 + > io_uring/rw.c | 20 ++ > 14 files changed, 967 insertions(+), 160 deletions(-) > create mode 100644 io_uring/fused_cmd.c > create mode 100644 io_uring/fused_cmd.h >
On Thu, Mar 16, 2023 at 11:13:39AM +0800, Xiaoguang Wang wrote: > hi, > > > Hello, > > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > and its ->issue() can retrieve/import buffer from master request's > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > submits slave OP just like normal OP issued from userspace, that said, > > SQE order is kept, and batching handling is done too. > Thanks for this great work, seems that we're now in the right direction > to support ublk zero copy, I believe this feature will improve io throughput > greatly and reduce ublk's cpu resource usage. > > I have gone through your 2th patch, and have some little concerns here: > Say we have one ublk loop target device, but it has 4 backend files, > every file will carry 25% of device capacity and it's implemented in stripped > way, then for every io request, current implementation will need issed 4 > fused_cmd, right? 4 slave sqes are necessary, but it would be better to > have just one master sqe, so I wonder whether we can have another Yeah, the current approach needs 4 fused command with 4 slave request, but from user viewpoint it is just 4 128byte SQEs. It is pretty lightweight to handle master command, just calling io_fused_cmd_provide_kbuf() for providing the buffer, so IMO it is fine to submit 4 fused command to handle single stripped IO. > method. The key point is to let io_uring support register various kernel > memory objects, which come from kernel, such as ITER_BVEC or > ITER_KVEC. so how about below actions: > 1. add a new infrastructure in io_uring, which will support to register > various kernel memory objects in it, this new infrastructure could be > maintained in a xarray structure, every memory objects in it will have > a unique id. This registration could be done in a ublk uring cmd, io_uring > offers registration interface. > 2. then any sqe can use these memory objects freely, so long as it > passes above unique id in sqe properly. > Above are just rough ideas, just for your reference. I'd rather not add more complexity from the beginning, and IMO probably it could be the most simple & generic way to handle it by single fused command, at least the buffer lifetime/ownership won't cross multiple OPs. Registering per-io buffer isn't free, Pavel actually mentioned the idea, basically: 1) one OP is for registering buffer 2) another OP is for un-registering buffer Then we still need 3+ OPs(SQEs) for handling single IO, not mention the buffer has to be stored in global(per-ctx) data structure, and you have to pay cost to read/write global data structure in IO fast path. In the case of 4 stripped underlying device, you still need 6 64byte SQEs for handling single io. But in future if we don't have other better candidates and fused command can't scale well, we can extend it or add new OPs for improving the multiple underlying devices, but so far, not see the problem. > > And current zero-copy method only supports raw data redirection, if Yeah. > ublk targets need to crc, compress, encrypt raw io requests' pages, > then we'll still need to copy block layer's io data to userspace daemon. Yes, zero copy can't cover all cases, that is why I add read/write interface to support other cases, see patch 14, then userspace can do whatwever they like. Actually once zero copy is accepted, I'd suggest to mark the non-zc code path as legacy, since the copy can be done explicitly in userspace by the added read()/write(). And ublk driver can get simplified & cleaned, same with userspace implementation. > In that way, ebpf may give a help :) we directly operate block layer's > io data in ebpf prog, doing crc or compress, encrypt, still does not need > to copy to userspace daemon. But as you said before, ebpf may not > support complicated user io logic, a much long way to go... Of course, there can be lots of work for future improvement, and ebpf is really one great weapon, but let's start effectively with something reliable & simple. thanks, Ming
On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > Hello, > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > and its ->issue() can retrieve/import buffer from master request's > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > submits slave OP just like normal OP issued from userspace, that said, > SQE order is kept, and batching handling is done too. > > Please see detailed design in commit log of the 2th patch, and one big > point is how to handle buffer ownership. > > With this way, it is easy to support zero copy for ublk/fuse device. > > Basically userspace can specify any sub-buffer of the ublk block request > buffer from the fused command just by setting 'offset/len' > in the slave SQE for running slave OP. This way is flexible to implement > io mapping: mirror, stripped, ... > > The 3th & 4th patches enable fused slave support for the following OPs: > > OP_READ/OP_WRITE > OP_SEND/OP_RECV/OP_SEND_ZC > > The other ublk patches cleans ublk driver and implement fused command > for supporting zero copy. > > Follows userspace code: > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > > ublk add -t [loop|nbd|qcow2] -z .... > > Basic fs mount/kernel building and builtin test are done, and also not > observe regression on xfstest test over ublk-loop with zero copy. > > Also add liburing test case for covering fused command based on miniublk > of blktest: > > https://github.com/ming1/liburing/commits/fused_cmd_miniublk > > Performance improvement is obvious on memory bandwidth > related workloads, such as, 1~2X improvement on 64K/512K BS > IO test on loop with ramfs backing file. > > Any comments are welcome! > > V3: > - fix build warning reported by kernel test robot > - drop patch for checking fused flags on existed drivers with > ->uring_command(), which isn't necessary, since we do not do that > when adding new ioctl or uring command > - inline io_init_rq() for core code, so just export io_init_slave_req > - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > will be cleared > - pass xfstest over ublk-loop Hello Jens and Guys, I have been working on io_uring zero copy support for ublk/fuse for a while, and I appreciate you may share any thoughts on this patchset or approach? Thanks, Ming
On 3/17/23 2:14?AM, Ming Lei wrote: > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: >> Hello, >> >> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >> and its ->issue() can retrieve/import buffer from master request's >> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >> submits slave OP just like normal OP issued from userspace, that said, >> SQE order is kept, and batching handling is done too. >> >> Please see detailed design in commit log of the 2th patch, and one big >> point is how to handle buffer ownership. >> >> With this way, it is easy to support zero copy for ublk/fuse device. >> >> Basically userspace can specify any sub-buffer of the ublk block request >> buffer from the fused command just by setting 'offset/len' >> in the slave SQE for running slave OP. This way is flexible to implement >> io mapping: mirror, stripped, ... >> >> The 3th & 4th patches enable fused slave support for the following OPs: >> >> OP_READ/OP_WRITE >> OP_SEND/OP_RECV/OP_SEND_ZC >> >> The other ublk patches cleans ublk driver and implement fused command >> for supporting zero copy. >> >> Follows userspace code: >> >> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 >> >> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: >> >> ublk add -t [loop|nbd|qcow2] -z .... >> >> Basic fs mount/kernel building and builtin test are done, and also not >> observe regression on xfstest test over ublk-loop with zero copy. >> >> Also add liburing test case for covering fused command based on miniublk >> of blktest: >> >> https://github.com/ming1/liburing/commits/fused_cmd_miniublk >> >> Performance improvement is obvious on memory bandwidth >> related workloads, such as, 1~2X improvement on 64K/512K BS >> IO test on loop with ramfs backing file. >> >> Any comments are welcome! >> >> V3: >> - fix build warning reported by kernel test robot >> - drop patch for checking fused flags on existed drivers with >> ->uring_command(), which isn't necessary, since we do not do that >> when adding new ioctl or uring command >> - inline io_init_rq() for core code, so just export io_init_slave_req >> - return result of failed slave request unconditionally since REQ_F_CQE_SKIP >> will be cleared >> - pass xfstest over ublk-loop > > Hello Jens and Guys, > > I have been working on io_uring zero copy support for ublk/fuse for a while, and > I appreciate you may share any thoughts on this patchset or approach? I'm a bit split on this one, as I really like (and want) the feature. ublk has become popular pretty quickly, and it makes a LOT of sense to support zero copy for it. At the same time, I'm not really a huge fan of the fused commands... They seem too specialized to be useful for other things, and it'd be a shame to do something like that only for it later to be replaced by a generic solution. And then we're stuck with supporting fused commands forever, not sure I like that prospect. Both Pavel and Xiaoguang voiced similar concerns, and I think it may be worth spending a bit more time on figuring out if splice can help us here. David Howells currently has a lot going on in that area too. So while I'd love to see this feature get queued up right now, I also don't want to prematurely do so. Can we split out the fixes from this series into a separate series that we can queue up now? That would also help shrink the patchset, which is always a win for review.
Hi Jens, Thanks for the response! On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > On 3/17/23 2:14?AM, Ming Lei wrote: > > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > >> Hello, > >> > >> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > >> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > >> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > >> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > >> and its ->issue() can retrieve/import buffer from master request's > >> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > >> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > >> submits slave OP just like normal OP issued from userspace, that said, > >> SQE order is kept, and batching handling is done too. > >> > >> Please see detailed design in commit log of the 2th patch, and one big > >> point is how to handle buffer ownership. > >> > >> With this way, it is easy to support zero copy for ublk/fuse device. > >> > >> Basically userspace can specify any sub-buffer of the ublk block request > >> buffer from the fused command just by setting 'offset/len' > >> in the slave SQE for running slave OP. This way is flexible to implement > >> io mapping: mirror, stripped, ... > >> > >> The 3th & 4th patches enable fused slave support for the following OPs: > >> > >> OP_READ/OP_WRITE > >> OP_SEND/OP_RECV/OP_SEND_ZC > >> > >> The other ublk patches cleans ublk driver and implement fused command > >> for supporting zero copy. > >> > >> Follows userspace code: > >> > >> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > >> > >> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > >> > >> ublk add -t [loop|nbd|qcow2] -z .... > >> > >> Basic fs mount/kernel building and builtin test are done, and also not > >> observe regression on xfstest test over ublk-loop with zero copy. > >> > >> Also add liburing test case for covering fused command based on miniublk > >> of blktest: > >> > >> https://github.com/ming1/liburing/commits/fused_cmd_miniublk > >> > >> Performance improvement is obvious on memory bandwidth > >> related workloads, such as, 1~2X improvement on 64K/512K BS > >> IO test on loop with ramfs backing file. > >> > >> Any comments are welcome! > >> > >> V3: > >> - fix build warning reported by kernel test robot > >> - drop patch for checking fused flags on existed drivers with > >> ->uring_command(), which isn't necessary, since we do not do that > >> when adding new ioctl or uring command > >> - inline io_init_rq() for core code, so just export io_init_slave_req > >> - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > >> will be cleared > >> - pass xfstest over ublk-loop > > > > Hello Jens and Guys, > > > > I have been working on io_uring zero copy support for ublk/fuse for a while, and > > I appreciate you may share any thoughts on this patchset or approach? > > I'm a bit split on this one, as I really like (and want) the feature. > ublk has become popular pretty quickly, and it makes a LOT of sense to > support zero copy for it. At the same time, I'm not really a huge fan of > the fused commands... They seem too specialized to be useful for other > things, and it'd be a shame to do something like that only for it later > to be replaced by a generic solution. And then we're stuck with > supporting fused commands forever, not sure I like that prospect. > > Both Pavel and Xiaoguang voiced similar concerns, and I think it may be > worth spending a bit more time on figuring out if splice can help us > here. David Howells currently has a lot going on in that area too. IMO, splice(->splice_read()) can help much less in this use case, and I can't see improvement David Howells has done in this area: 1) we need to pass reference of the whole buffer from driver to io_uring, which is missed in splice, which just deals with page reference; for passing whole buffer reference, we have to apply per buffer pipe to solve the problem, and this way is expensive since the pipe can't be freed until all buffers are consumed. 2) reference can't outlive the whole buffer, and splice still misses mechanism to provide such guarantee; splice can just make sure that page won't be gone if page reference is grabbed, but here we care more the whole buffer & its (shared)references lifetime 3) current ->splice_read() misses capability to provide writeable reference to spliced page[2]; either we have to pass new flags to ->splice_read() or passing back new pipe buf flags, unfortunately Linus thought it isn't good to extend pipe/splice for such purpose, and now I agree with Linus now. I believe that Pavel has realized this point[3] too, and here the only of value of using pipe is to reuse ->splice_read(), however, the above points show that ->splice_read() isn't good at this purpose. [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ [3] https://lore.kernel.org/linux-block/7cdea685-98d3-e24d-8282-87cb44ae6174@gmail.com/ > > So while I'd love to see this feature get queued up right now, I also > don't want to prematurely do so. Can we split out the fixes from this > series into a separate series that we can queue up now? That would also > help shrink the patchset, which is always a win for review. There is only one fix(patch 5), and the real part is actually the 1st 4 patches. I will separate patch 5 from the whole patchset and send out soon, and will post out this patchset v4 by improving document for explaining how fused command solves this problem in one safe & efficient way. thanks, Ming
On 3/18/23 7:35?AM, Ming Lei wrote: > Hi Jens, > > Thanks for the response! > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: >> On 3/17/23 2:14?AM, Ming Lei wrote: >>> On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: >>>> Hello, >>>> >>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>>> and its ->issue() can retrieve/import buffer from master request's >>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>>> submits slave OP just like normal OP issued from userspace, that said, >>>> SQE order is kept, and batching handling is done too. >>>> >>>> Please see detailed design in commit log of the 2th patch, and one big >>>> point is how to handle buffer ownership. >>>> >>>> With this way, it is easy to support zero copy for ublk/fuse device. >>>> >>>> Basically userspace can specify any sub-buffer of the ublk block request >>>> buffer from the fused command just by setting 'offset/len' >>>> in the slave SQE for running slave OP. This way is flexible to implement >>>> io mapping: mirror, stripped, ... >>>> >>>> The 3th & 4th patches enable fused slave support for the following OPs: >>>> >>>> OP_READ/OP_WRITE >>>> OP_SEND/OP_RECV/OP_SEND_ZC >>>> >>>> The other ublk patches cleans ublk driver and implement fused command >>>> for supporting zero copy. >>>> >>>> Follows userspace code: >>>> >>>> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 >>>> >>>> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: >>>> >>>> ublk add -t [loop|nbd|qcow2] -z .... >>>> >>>> Basic fs mount/kernel building and builtin test are done, and also not >>>> observe regression on xfstest test over ublk-loop with zero copy. >>>> >>>> Also add liburing test case for covering fused command based on miniublk >>>> of blktest: >>>> >>>> https://github.com/ming1/liburing/commits/fused_cmd_miniublk >>>> >>>> Performance improvement is obvious on memory bandwidth >>>> related workloads, such as, 1~2X improvement on 64K/512K BS >>>> IO test on loop with ramfs backing file. >>>> >>>> Any comments are welcome! >>>> >>>> V3: >>>> - fix build warning reported by kernel test robot >>>> - drop patch for checking fused flags on existed drivers with >>>> ->uring_command(), which isn't necessary, since we do not do that >>>> when adding new ioctl or uring command >>>> - inline io_init_rq() for core code, so just export io_init_slave_req >>>> - return result of failed slave request unconditionally since REQ_F_CQE_SKIP >>>> will be cleared >>>> - pass xfstest over ublk-loop >>> >>> Hello Jens and Guys, >>> >>> I have been working on io_uring zero copy support for ublk/fuse for a while, and >>> I appreciate you may share any thoughts on this patchset or approach? >> >> I'm a bit split on this one, as I really like (and want) the feature. >> ublk has become popular pretty quickly, and it makes a LOT of sense to >> support zero copy for it. At the same time, I'm not really a huge fan of >> the fused commands... They seem too specialized to be useful for other >> things, and it'd be a shame to do something like that only for it later >> to be replaced by a generic solution. And then we're stuck with >> supporting fused commands forever, not sure I like that prospect. >> >> Both Pavel and Xiaoguang voiced similar concerns, and I think it may be >> worth spending a bit more time on figuring out if splice can help us >> here. David Howells currently has a lot going on in that area too. > > IMO, splice(->splice_read()) can help much less in this use case, and > I can't see improvement David Howells has done in this area: > > 1) we need to pass reference of the whole buffer from driver to io_uring, > which is missed in splice, which just deals with page reference; for > passing whole buffer reference, we have to apply per buffer pipe to > solve the problem, and this way is expensive since the pipe can't > be freed until all buffers are consumed. > > 2) reference can't outlive the whole buffer, and splice still misses > mechanism to provide such guarantee; splice can just make sure that > page won't be gone if page reference is grabbed, but here we care > more the whole buffer & its (shared)references lifetime > > 3) current ->splice_read() misses capability to provide writeable > reference to spliced page[2]; either we have to pass new flags > to ->splice_read() or passing back new pipe buf flags, unfortunately > Linus thought it isn't good to extend pipe/splice for such purpose, > and now I agree with Linus now. > > I believe that Pavel has realized this point[3] too, and here the only > of value of using pipe is to reuse ->splice_read(), however, the above > points show that ->splice_read() isn't good at this purpose. > > > [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ > [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ > [3] https://lore.kernel.org/linux-block/7cdea685-98d3-e24d-8282-87cb44ae6174@gmail.com/ splice is just one idea, but I do wonder if there's a way to express this relationship (and buffer handovers) that doesn't involve needing these odd kind of fused commands where they must be submitted as one big sqe, but really are two normal ones. BPF is obviously one way, and maybe we'll do BPF with io_uring at some point, but it makes things rather more complicated to use and I'd prefer to avoid it if we can. I'll take a closer look at the patches. >> So while I'd love to see this feature get queued up right now, I also >> don't want to prematurely do so. Can we split out the fixes from this >> series into a separate series that we can queue up now? That would also >> help shrink the patchset, which is always a win for review. > > There is only one fix(patch 5), and the real part is actually the 1st 4 > patches. > > I will separate patch 5 from the whole patchset and send out soon, and will > post out this patchset v4 by improving document for explaining how fused > command solves this problem in one safe & efficient way. Thanks, did get that one now and applied it.
On Sat, Mar 18, 2023 at 08:36:37AM -0600, Jens Axboe wrote: > On 3/18/23 7:35?AM, Ming Lei wrote: > > Hi Jens, > > > > Thanks for the response! > > > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > >> On 3/17/23 2:14?AM, Ming Lei wrote: > >>> On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > >>>> Hello, > >>>> > >>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > >>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > >>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > >>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > >>>> and its ->issue() can retrieve/import buffer from master request's > >>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > >>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > >>>> submits slave OP just like normal OP issued from userspace, that said, > >>>> SQE order is kept, and batching handling is done too. > >>>> > >>>> Please see detailed design in commit log of the 2th patch, and one big > >>>> point is how to handle buffer ownership. > >>>> > >>>> With this way, it is easy to support zero copy for ublk/fuse device. > >>>> > >>>> Basically userspace can specify any sub-buffer of the ublk block request > >>>> buffer from the fused command just by setting 'offset/len' > >>>> in the slave SQE for running slave OP. This way is flexible to implement > >>>> io mapping: mirror, stripped, ... > >>>> > >>>> The 3th & 4th patches enable fused slave support for the following OPs: > >>>> > >>>> OP_READ/OP_WRITE > >>>> OP_SEND/OP_RECV/OP_SEND_ZC > >>>> > >>>> The other ublk patches cleans ublk driver and implement fused command > >>>> for supporting zero copy. > >>>> > >>>> Follows userspace code: > >>>> > >>>> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > >>>> > >>>> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > >>>> > >>>> ublk add -t [loop|nbd|qcow2] -z .... > >>>> > >>>> Basic fs mount/kernel building and builtin test are done, and also not > >>>> observe regression on xfstest test over ublk-loop with zero copy. > >>>> > >>>> Also add liburing test case for covering fused command based on miniublk > >>>> of blktest: > >>>> > >>>> https://github.com/ming1/liburing/commits/fused_cmd_miniublk > >>>> > >>>> Performance improvement is obvious on memory bandwidth > >>>> related workloads, such as, 1~2X improvement on 64K/512K BS > >>>> IO test on loop with ramfs backing file. > >>>> > >>>> Any comments are welcome! > >>>> > >>>> V3: > >>>> - fix build warning reported by kernel test robot > >>>> - drop patch for checking fused flags on existed drivers with > >>>> ->uring_command(), which isn't necessary, since we do not do that > >>>> when adding new ioctl or uring command > >>>> - inline io_init_rq() for core code, so just export io_init_slave_req > >>>> - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > >>>> will be cleared > >>>> - pass xfstest over ublk-loop > >>> > >>> Hello Jens and Guys, > >>> > >>> I have been working on io_uring zero copy support for ublk/fuse for a while, and > >>> I appreciate you may share any thoughts on this patchset or approach? > >> > >> I'm a bit split on this one, as I really like (and want) the feature. > >> ublk has become popular pretty quickly, and it makes a LOT of sense to > >> support zero copy for it. At the same time, I'm not really a huge fan of > >> the fused commands... They seem too specialized to be useful for other > >> things, and it'd be a shame to do something like that only for it later > >> to be replaced by a generic solution. And then we're stuck with > >> supporting fused commands forever, not sure I like that prospect. > >> > >> Both Pavel and Xiaoguang voiced similar concerns, and I think it may be > >> worth spending a bit more time on figuring out if splice can help us > >> here. David Howells currently has a lot going on in that area too. > > > > IMO, splice(->splice_read()) can help much less in this use case, and > > I can't see improvement David Howells has done in this area: > > > > 1) we need to pass reference of the whole buffer from driver to io_uring, > > which is missed in splice, which just deals with page reference; for > > passing whole buffer reference, we have to apply per buffer pipe to > > solve the problem, and this way is expensive since the pipe can't > > be freed until all buffers are consumed. > > > > 2) reference can't outlive the whole buffer, and splice still misses > > mechanism to provide such guarantee; splice can just make sure that > > page won't be gone if page reference is grabbed, but here we care > > more the whole buffer & its (shared)references lifetime > > > > 3) current ->splice_read() misses capability to provide writeable > > reference to spliced page[2]; either we have to pass new flags > > to ->splice_read() or passing back new pipe buf flags, unfortunately > > Linus thought it isn't good to extend pipe/splice for such purpose, > > and now I agree with Linus now. > > > > I believe that Pavel has realized this point[3] too, and here the only > > of value of using pipe is to reuse ->splice_read(), however, the above > > points show that ->splice_read() isn't good at this purpose. > > > > > > [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ > > [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ > > [3] https://lore.kernel.org/linux-block/7cdea685-98d3-e24d-8282-87cb44ae6174@gmail.com/ > > splice is just one idea, but I do wonder if there's a way to express > this relationship (and buffer handovers) that doesn't involve needing > these odd kind of fused commands where they must be submitted as one big > sqe, but really are two normal ones. BPF is obviously one way, and maybe The problem can't be solved in single normal SQE, so either two normal SQEs or single big one. I thought of using two SQEs: 1) the 1st SQE has to be the uring command for providing buffer by reference 2) the 2nd one consumes the buffer reference, so it depends on 1st SQE 3) the 1st SQE has to be completed after the 2nd one is done because reference(io_uring_bvec_buf) can't outlive value(buffer, or bvec), so existed IO_LINK can't handle this problem simply That is why I take single big SQE, which can: 1) meet buffer & its reference lifetime requirement 2) dependence between uring command for providing buffer and normal OP which consumes the provided buffer reference > we'll do BPF with io_uring at some point, but it makes things rather > more complicated to use and I'd prefer to avoid it if we can. Agree. > > I'll take a closer look at the patches. Thanks!
On 3/14/23 6:57?AM, Ming Lei wrote: > Basically userspace can specify any sub-buffer of the ublk block request > buffer from the fused command just by setting 'offset/len' > in the slave SQE for running slave OP. This way is flexible to implement > io mapping: mirror, stripped, ... > > The 3th & 4th patches enable fused slave support for the following OPs: > > OP_READ/OP_WRITE > OP_SEND/OP_RECV/OP_SEND_ZC > > The other ublk patches cleans ublk driver and implement fused command > for supporting zero copy. > > Follows userspace code: > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 Ran some quick testing here with qcow2. This is just done on my laptop in kvm, so take them with a grain of salt, results may be better elsewhere. Basline: 64k reads 98-100K IOPS 6-6.1GB/sec (ublk 100%, io_uring 9%) 4k reads 670-680K IOPS 2.6GB/sec (ublk 65%, io_uring 44%) and with zerocopy enabled: 64k reads 184K IOPS 11.5GB/sec (ublk 91%, io_uring 12%) 4k reads 730K IOPS 2.8GB/sec (ublk 73%, io_uring 48%) and with zerocopy and using SINGLE_ISSUER|COOP_TASKRUN for the ring: 64k reads 205K IOPS 12.8GB/sec (ublk 91%, io_uring 12%) 4k reads 730K IOPS 2.8GB/sec (ublk 66%, io_uring 42%) Don't put too much into the CPU utilization numbers, they are just indicative and not super accurate. But overall a nice win for larger block sizes with zero copy. We seem to be IOPS limited on this particular setup, which is most likely why 4k isn't showing any major wins here. Eg running 8k with zero copy, I get the same IOPS limit, just obviously doubling the bandwidth of the 4k run: IOPS=732.26K, BW=5.72GiB/s, IOS/call=32/32 IOPS=733.38K, BW=5.73GiB/s, IOS/call=32/32 I also tried using DEFER_TASKRUN, but it stalls on setup. Most likely something trivial, didn't poke any further at that.
On 3/16/23 03:13, Xiaoguang Wang wrote: >> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >> and its ->issue() can retrieve/import buffer from master request's >> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >> submits slave OP just like normal OP issued from userspace, that said, >> SQE order is kept, and batching handling is done too. > Thanks for this great work, seems that we're now in the right direction > to support ublk zero copy, I believe this feature will improve io throughput > greatly and reduce ublk's cpu resource usage. > > I have gone through your 2th patch, and have some little concerns here: > Say we have one ublk loop target device, but it has 4 backend files, > every file will carry 25% of device capacity and it's implemented in stripped > way, then for every io request, current implementation will need issed 4 > fused_cmd, right? 4 slave sqes are necessary, but it would be better to > have just one master sqe, so I wonder whether we can have another > method. The key point is to let io_uring support register various kernel > memory objects, which come from kernel, such as ITER_BVEC or > ITER_KVEC. so how about below actions: > 1. add a new infrastructure in io_uring, which will support to register > various kernel memory objects in it, this new infrastructure could be > maintained in a xarray structure, every memory objects in it will have > a unique id. This registration could be done in a ublk uring cmd, io_uring > offers registration interface. > 2. then any sqe can use these memory objects freely, so long as it > passes above unique id in sqe properly. > Above are just rough ideas, just for your reference. It precisely hints on what I proposed a bit earlier, that makes me not alone thinking that it's a good idea to have a design allowing 1) multiple ops using a buffer and 2) to limiting it to one single submission because the userspace might want to preprocess a part of the data, multiplex it or on the opposite divide. I was mostly coming from non ublk cases, and one example would be such zc recv, parsing the app level headers and redirecting the rest of the data somewhere. I haven't got a chance to work on it but will return to it in a week. The discussion was here: https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/
On Sat, Mar 18, 2023 at 04:23:35PM +0000, Pavel Begunkov wrote: > On 3/16/23 03:13, Xiaoguang Wang wrote: > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > > and its ->issue() can retrieve/import buffer from master request's > > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > > submits slave OP just like normal OP issued from userspace, that said, > > > SQE order is kept, and batching handling is done too. > > Thanks for this great work, seems that we're now in the right direction > > to support ublk zero copy, I believe this feature will improve io throughput > > greatly and reduce ublk's cpu resource usage. > > > > I have gone through your 2th patch, and have some little concerns here: > > Say we have one ublk loop target device, but it has 4 backend files, > > every file will carry 25% of device capacity and it's implemented in stripped > > way, then for every io request, current implementation will need issed 4 > > fused_cmd, right? 4 slave sqes are necessary, but it would be better to > > have just one master sqe, so I wonder whether we can have another > > method. The key point is to let io_uring support register various kernel > > memory objects, which come from kernel, such as ITER_BVEC or > > ITER_KVEC. so how about below actions: > > 1. add a new infrastructure in io_uring, which will support to register > > various kernel memory objects in it, this new infrastructure could be > > maintained in a xarray structure, every memory objects in it will have > > a unique id. This registration could be done in a ublk uring cmd, io_uring > > offers registration interface. > > 2. then any sqe can use these memory objects freely, so long as it > > passes above unique id in sqe properly. > > Above are just rough ideas, just for your reference. > > It precisely hints on what I proposed a bit earlier, that makes > me not alone thinking that it's a good idea to have a design allowing > 1) multiple ops using a buffer and Firstly fused command does cover this case, io_fused_cmd_provide_kbuf() is very cheap, which just passes buffer reference. Secondly, your original suggestion is to wire the per-io buffer with context fixed buffer, which basically has to add two OPs: 1) one for registering buffer 2) another one for un-registering buffer So one usual such IO may have to takes 3+ SQEs, which won't be efficient for single or even double submission cases since the cost of touching global context fixed buffer can't be ignored. > 2) to limiting it to one single > submission because the userspace might want to preprocess a part > of the data, multiplex it or on the opposite divide. Unfortunately ublk has to support multiple submissions, and there can be lots of such use cases, logical volume manager(mirror, stripped), distributed network storage, ... Thanks, Ming
On 3/18/23 13:35, Ming Lei wrote: > Hi Jens, > > Thanks for the response! > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: >> On 3/17/23 2:14?AM, Ming Lei wrote: >>> On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: >>>> Hello, >>>> >>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>>> and its ->issue() can retrieve/import buffer from master request's >>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>>> submits slave OP just like normal OP issued from userspace, that said, >>>> SQE order is kept, and batching handling is done too. >>>> >>>> Please see detailed design in commit log of the 2th patch, and one big >>>> point is how to handle buffer ownership. >>>> >>>> With this way, it is easy to support zero copy for ublk/fuse device. >>>> >>>> Basically userspace can specify any sub-buffer of the ublk block request >>>> buffer from the fused command just by setting 'offset/len' >>>> in the slave SQE for running slave OP. This way is flexible to implement >>>> io mapping: mirror, stripped, ... >>>> >>>> The 3th & 4th patches enable fused slave support for the following OPs: >>>> >>>> OP_READ/OP_WRITE >>>> OP_SEND/OP_RECV/OP_SEND_ZC >>>> >>>> The other ublk patches cleans ublk driver and implement fused command >>>> for supporting zero copy. >>>> >>>> Follows userspace code: >>>> >>>> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 >>>> >>>> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: >>>> >>>> ublk add -t [loop|nbd|qcow2] -z .... >>>> >>>> Basic fs mount/kernel building and builtin test are done, and also not >>>> observe regression on xfstest test over ublk-loop with zero copy. >>>> >>>> Also add liburing test case for covering fused command based on miniublk >>>> of blktest: >>>> >>>> https://github.com/ming1/liburing/commits/fused_cmd_miniublk >>>> >>>> Performance improvement is obvious on memory bandwidth >>>> related workloads, such as, 1~2X improvement on 64K/512K BS >>>> IO test on loop with ramfs backing file. >>>> >>>> Any comments are welcome! >>>> >>>> V3: >>>> - fix build warning reported by kernel test robot >>>> - drop patch for checking fused flags on existed drivers with >>>> ->uring_command(), which isn't necessary, since we do not do that >>>> when adding new ioctl or uring command >>>> - inline io_init_rq() for core code, so just export io_init_slave_req >>>> - return result of failed slave request unconditionally since REQ_F_CQE_SKIP >>>> will be cleared >>>> - pass xfstest over ublk-loop >>> >>> Hello Jens and Guys, >>> >>> I have been working on io_uring zero copy support for ublk/fuse for a while, and >>> I appreciate you may share any thoughts on this patchset or approach? >> >> I'm a bit split on this one, as I really like (and want) the feature. >> ublk has become popular pretty quickly, and it makes a LOT of sense to >> support zero copy for it. At the same time, I'm not really a huge fan of >> the fused commands... They seem too specialized to be useful for other >> things, and it'd be a shame to do something like that only for it later >> to be replaced by a generic solution. And then we're stuck with >> supporting fused commands forever, not sure I like that prospect. >> >> Both Pavel and Xiaoguang voiced similar concerns, and I think it may be >> worth spending a bit more time on figuring out if splice can help us >> here. David Howells currently has a lot going on in that area too. > > IMO, splice(->splice_read()) can help much less in this use case, and > I can't see improvement David Howells has done in this area: Let me correct a misunderstanding I've seen a couple of times from people. Apart from the general idea of providing buffers, it's not that bound to splice. Yes, I reused splicing guts for that half-made POC, but we can add a new callback that would do it a bit nicer, i.e. better consolidating returned buffers. Would probably be even better to have both of them falling back to splice so it can cover more cases. The core of it is mediating buffers through io_uring's registered buffer table, which decouples all the components from each other. > 1) we need to pass reference of the whole buffer from driver to io_uring, > which is missed in splice, which just deals with page reference; for > passing whole buffer reference, we have to apply per buffer pipe to > solve the problem, and this way is expensive since the pipe can't > be freed until all buffers are consumed. > > 2) reference can't outlive the whole buffer, and splice still misses > mechanism to provide such guarantee; splice can just make sure that > page won't be gone if page reference is grabbed, but here we care > more the whole buffer & its (shared)references lifetime > > 3) current ->splice_read() misses capability to provide writeable > reference to spliced page[2]; either we have to pass new flags > to ->splice_read() or passing back new pipe buf flags, unfortunately > Linus thought it isn't good to extend pipe/splice for such purpose, > and now I agree with Linus now. It might be a non-workable option if we're thinking about splice(2) and pipes, but pipes and ->splice_read() are just internal details, an execution mechanism, and it's hidden from the userspace. I guess someone might make a point that we don't want any changes to the splice code even if it doesn't affect splice(2) userspace users, but that's rather a part of development process. > I believe that Pavel has realized this point[3] too, and here the only > of value of using pipe is to reuse ->splice_read(), however, the above > points show that ->splice_read() isn't good at this purpose. But agree that, ->splice_read() doesn't support the revers direction, i.e. a file (e.g. ublk) provides buffers for someone to write into it, that would need to be extended in some way. > [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ Oops, missed this one > [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ Miklos said that it's better to signal the owner of buffer about completion, IIUC the way I was proposing, i.e. calling ->release when io_uring removes the buffer and all io_uring requests using it complete, should do exactly that. > [3] https://lore.kernel.org/linux-block/7cdea685-98d3-e24d-8282-87cb44ae6174@gmail.com/ > >> >> So while I'd love to see this feature get queued up right now, I also >> don't want to prematurely do so. Can we split out the fixes from this >> series into a separate series that we can queue up now? That would also >> help shrink the patchset, which is always a win for review. > > There is only one fix(patch 5), and the real part is actually the 1st 4 > patches. > > I will separate patch 5 from the whole patchset and send out soon, and will > post out this patchset v4 by improving document for explaining how fused > command solves this problem in one safe & efficient way.
On Sat, Mar 18, 2023 at 10:09:52AM -0600, Jens Axboe wrote: > On 3/14/23 6:57?AM, Ming Lei wrote: > > Basically userspace can specify any sub-buffer of the ublk block request > > buffer from the fused command just by setting 'offset/len' > > in the slave SQE for running slave OP. This way is flexible to implement > > io mapping: mirror, stripped, ... > > > > The 3th & 4th patches enable fused slave support for the following OPs: > > > > OP_READ/OP_WRITE > > OP_SEND/OP_RECV/OP_SEND_ZC > > > > The other ublk patches cleans ublk driver and implement fused command > > for supporting zero copy. > > > > Follows userspace code: > > > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > Ran some quick testing here with qcow2. This is just done on my laptop > in kvm, so take them with a grain of salt, results may be better > elsewhere. > > Basline: > > 64k reads 98-100K IOPS 6-6.1GB/sec (ublk 100%, io_uring 9%) > 4k reads 670-680K IOPS 2.6GB/sec (ublk 65%, io_uring 44%) > > and with zerocopy enabled: > > 64k reads 184K IOPS 11.5GB/sec (ublk 91%, io_uring 12%) > 4k reads 730K IOPS 2.8GB/sec (ublk 73%, io_uring 48%) There are other ways to observe the boost: 1) loop over file in tmpfs - 1~2X in my test 2) nbd with local nbd server(nbdkit memory ) - less than 1X in my test 3) null - which won't call into fused command, but can evaluate page copy cost - 5+X in my test > > and with zerocopy and using SINGLE_ISSUER|COOP_TASKRUN for the ring: > > 64k reads 205K IOPS 12.8GB/sec (ublk 91%, io_uring 12%) > 4k reads 730K IOPS 2.8GB/sec (ublk 66%, io_uring 42%) Looks SINGLE_ISSUER|COOP_TASKRUN can get ~10% improvement, will look it. Thanks, Ming
On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: > On 3/18/23 13:35, Ming Lei wrote: > > Hi Jens, > > > > Thanks for the response! > > > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > > > On 3/17/23 2:14?AM, Ming Lei wrote: > > > > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > > > > > Hello, > > > > > > > > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > > > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > > > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > > > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > > > > and its ->issue() can retrieve/import buffer from master request's > > > > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > > > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > > > > submits slave OP just like normal OP issued from userspace, that said, > > > > > SQE order is kept, and batching handling is done too. > > > > > > > > > > Please see detailed design in commit log of the 2th patch, and one big > > > > > point is how to handle buffer ownership. > > > > > > > > > > With this way, it is easy to support zero copy for ublk/fuse device. > > > > > > > > > > Basically userspace can specify any sub-buffer of the ublk block request > > > > > buffer from the fused command just by setting 'offset/len' > > > > > in the slave SQE for running slave OP. This way is flexible to implement > > > > > io mapping: mirror, stripped, ... > > > > > > > > > > The 3th & 4th patches enable fused slave support for the following OPs: > > > > > > > > > > OP_READ/OP_WRITE > > > > > OP_SEND/OP_RECV/OP_SEND_ZC > > > > > > > > > > The other ublk patches cleans ublk driver and implement fused command > > > > > for supporting zero copy. > > > > > > > > > > Follows userspace code: > > > > > > > > > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > > > > > > > > > All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > > > > > > > > > > ublk add -t [loop|nbd|qcow2] -z .... > > > > > > > > > > Basic fs mount/kernel building and builtin test are done, and also not > > > > > observe regression on xfstest test over ublk-loop with zero copy. > > > > > > > > > > Also add liburing test case for covering fused command based on miniublk > > > > > of blktest: > > > > > > > > > > https://github.com/ming1/liburing/commits/fused_cmd_miniublk > > > > > > > > > > Performance improvement is obvious on memory bandwidth > > > > > related workloads, such as, 1~2X improvement on 64K/512K BS > > > > > IO test on loop with ramfs backing file. > > > > > > > > > > Any comments are welcome! > > > > > > > > > > V3: > > > > > - fix build warning reported by kernel test robot > > > > > - drop patch for checking fused flags on existed drivers with > > > > > ->uring_command(), which isn't necessary, since we do not do that > > > > > when adding new ioctl or uring command > > > > > - inline io_init_rq() for core code, so just export io_init_slave_req > > > > > - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > > > > > will be cleared > > > > > - pass xfstest over ublk-loop > > > > > > > > Hello Jens and Guys, > > > > > > > > I have been working on io_uring zero copy support for ublk/fuse for a while, and > > > > I appreciate you may share any thoughts on this patchset or approach? > > > > > > I'm a bit split on this one, as I really like (and want) the feature. > > > ublk has become popular pretty quickly, and it makes a LOT of sense to > > > support zero copy for it. At the same time, I'm not really a huge fan of > > > the fused commands... They seem too specialized to be useful for other > > > things, and it'd be a shame to do something like that only for it later > > > to be replaced by a generic solution. And then we're stuck with > > > supporting fused commands forever, not sure I like that prospect. > > > > > > Both Pavel and Xiaoguang voiced similar concerns, and I think it may be > > > worth spending a bit more time on figuring out if splice can help us > > > here. David Howells currently has a lot going on in that area too. > > > > IMO, splice(->splice_read()) can help much less in this use case, and > > I can't see improvement David Howells has done in this area: > > Let me correct a misunderstanding I've seen a couple of times > from people. Apart from the general idea of providing buffers, it's > not that bound to splice. Yes, I reused splicing guts for that > half-made POC, but we can add a new callback that would do it a > bit nicer, i.e. better consolidating returned buffers. Would ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). > probably be even better to have both of them falling back to > splice so it can cover more cases. The core of it is mediating > buffers through io_uring's registered buffer table, which > decouples all the components from each other. For using pipe buffer's ->release() to release the whole buffer's reference, you have to allocate one pipe for each fixed buffer, and add pipe buffer to it, and keep each pipe buffer into the pipe until it is consumed, since ->release() needs to be called when unregistering buffer(all IOs are completed) It(allocating/free pipe node, and populating it with each page) is really inefficient for handling one single IO. So re-using splice for this purpose is still bad not mention splice can't support writeable spliced page. Wiring device io buffer with context registered buffer table looks like another approach, however: 1) two uring command OPs for registering/unregistering this buffer in io fast path has to be added since only userspace can know when buffer(reference) isn't needed 2) userspace becomes more complicated, 3+ OPs are required for handling one single device IO 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) for unregistering buffer 4) here the case is totally different with io_mapped_ubuf which isn't related to any specific file, and just belong to io_uring context; however, the device io buffer belongs to device(file) actually, so in theory it is wrong to put it into context's registered buffer table, and supposed to put into per-file buffer table which isn't supported by io_uring, or it becomes hard to implement multiple-device io buffer in single context since 'file + buffer key' has to be used to retrieve this buffer, probably xarray has to be relied, but - here the index is (file, buffer key) if the table is per-context, current xarray only supports index with type of 'unsigned long', so looks not doable - or per-file xarray has to be used, then the implementation becomes more complicated - write to xarray has to be done two times in fast io path, so another factor which hurts performance. > > > 1) we need to pass reference of the whole buffer from driver to io_uring, > > which is missed in splice, which just deals with page reference; for > > passing whole buffer reference, we have to apply per buffer pipe to > > solve the problem, and this way is expensive since the pipe can't > > be freed until all buffers are consumed. > > > > 2) reference can't outlive the whole buffer, and splice still misses > > mechanism to provide such guarantee; splice can just make sure that > > page won't be gone if page reference is grabbed, but here we care > > more the whole buffer & its (shared)references lifetime > > > > 3) current ->splice_read() misses capability to provide writeable > > reference to spliced page[2]; either we have to pass new flags > > to ->splice_read() or passing back new pipe buf flags, unfortunately > > Linus thought it isn't good to extend pipe/splice for such purpose, > > and now I agree with Linus now. > > It might be a non-workable option if we're thinking about splice(2) > and pipes, but pipes and ->splice_read() are just internal details, > an execution mechanism, and it's hidden from the userspace. both pipe and ->splice_read() are really exposed to userspace, and are used in other non-io_uring situations, so any change can not break existed splice/pipe usage, maybe I misunderstand your point? > > I guess someone might make a point that we don't want any changes > to the splice code even if it doesn't affect splice(2) userspace > users, but that's rather a part of development process. > > I believe that Pavel has realized this point[3] too, and here the only > > of value of using pipe is to reuse ->splice_read(), however, the above > > points show that ->splice_read() isn't good at this purpose. > > But agree that, ->splice_read() doesn't support the revers > direction, i.e. a file (e.g. ublk) provides buffers for > someone to write into it, that would need to be extended > in some way. Linus has objected[1] explicitly to extend it in this way: There's no point trying to deal with "if unexpectedly doing crazy things". If a sink writes the data, the sinkm is so unbelievably buggy that it's not even funny. [1] https://lore.kernel.org/linux-block/CAHk-=wgJsi7t7YYpuo6ewXGnHz2nmj67iWR6KPGoz5TBu34mWQ@mail.gmail.com/ That is also the reason why fuse can only support write zero copy via splice for 10+ years. > > > [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ > > Oops, missed this one > > > [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ > > Miklos said that it's better to signal the owner of buffer about > completion, IIUC the way I was proposing, i.e. calling ->release > when io_uring removes the buffer and all io_uring requests using > it complete, should do exactly that. ->release() just for acking the page consumption, what the ublk needs is to drop the whole buffer(represented by bvec) reference when the buffer isn't used by normal OPs, actually similar with fuse's case, because buffer reference can't outlive the buffer itself(repesented by bvec). Yeah, probably releasing whole buffer reference can be done by ->release() in very complicated way, but the whole pipe & pipe buffer has to be kept in the whole IO lifetime for calling each pipe buffer's ->release(), so you have to allocate one pipe when registering this buffer, and release it when un-registering it. Much less efficient. In short, splice can't help us for meeting ublk/fuse requirement. Thanks, Ming
On Sun, Mar 19, 2023 at 07:42:26AM +0800, Ming Lei wrote: > On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: > > On 3/18/23 13:35, Ming Lei wrote: > > > Hi Jens, > > > > > > Thanks for the response! > > > > > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > > > > On 3/17/23 2:14?AM, Ming Lei wrote: > > > > > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > > > > > > Hello, > > > > > > > > > > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > > > > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > > > > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > > > > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > > > > > and its ->issue() can retrieve/import buffer from master request's > > > > > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > > > > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > > > > > submits slave OP just like normal OP issued from userspace, that said, > > > > > > SQE order is kept, and batching handling is done too. > > > > > > > > > > > > Please see detailed design in commit log of the 2th patch, and one big > > > > > > point is how to handle buffer ownership. > > > > > > > > > > > > With this way, it is easy to support zero copy for ublk/fuse device. > > > > > > > > > > > > Basically userspace can specify any sub-buffer of the ublk block request > > > > > > buffer from the fused command just by setting 'offset/len' > > > > > > in the slave SQE for running slave OP. This way is flexible to implement > > > > > > io mapping: mirror, stripped, ... > > > > > > > > > > > > The 3th & 4th patches enable fused slave support for the following OPs: > > > > > > > > > > > > OP_READ/OP_WRITE > > > > > > OP_SEND/OP_RECV/OP_SEND_ZC > > > > > > > > > > > > The other ublk patches cleans ublk driver and implement fused command > > > > > > for supporting zero copy. > > > > > > > > > > > > Follows userspace code: > > > > > > > > > > > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > > > > > > > > > > > All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > > > > > > > > > > > > ublk add -t [loop|nbd|qcow2] -z .... > > > > > > > > > > > > Basic fs mount/kernel building and builtin test are done, and also not > > > > > > observe regression on xfstest test over ublk-loop with zero copy. > > > > > > > > > > > > Also add liburing test case for covering fused command based on miniublk > > > > > > of blktest: > > > > > > > > > > > > https://github.com/ming1/liburing/commits/fused_cmd_miniublk > > > > > > > > > > > > Performance improvement is obvious on memory bandwidth > > > > > > related workloads, such as, 1~2X improvement on 64K/512K BS > > > > > > IO test on loop with ramfs backing file. > > > > > > > > > > > > Any comments are welcome! > > > > > > > > > > > > V3: > > > > > > - fix build warning reported by kernel test robot > > > > > > - drop patch for checking fused flags on existed drivers with > > > > > > ->uring_command(), which isn't necessary, since we do not do that > > > > > > when adding new ioctl or uring command > > > > > > - inline io_init_rq() for core code, so just export io_init_slave_req > > > > > > - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > > > > > > will be cleared > > > > > > - pass xfstest over ublk-loop > > > > > > > > > > Hello Jens and Guys, > > > > > > > > > > I have been working on io_uring zero copy support for ublk/fuse for a while, and > > > > > I appreciate you may share any thoughts on this patchset or approach? > > > > > > > > I'm a bit split on this one, as I really like (and want) the feature. > > > > ublk has become popular pretty quickly, and it makes a LOT of sense to > > > > support zero copy for it. At the same time, I'm not really a huge fan of > > > > the fused commands... They seem too specialized to be useful for other > > > > things, and it'd be a shame to do something like that only for it later > > > > to be replaced by a generic solution. And then we're stuck with > > > > supporting fused commands forever, not sure I like that prospect. > > > > > > > > Both Pavel and Xiaoguang voiced similar concerns, and I think it may be > > > > worth spending a bit more time on figuring out if splice can help us > > > > here. David Howells currently has a lot going on in that area too. > > > > > > IMO, splice(->splice_read()) can help much less in this use case, and > > > I can't see improvement David Howells has done in this area: > > > > Let me correct a misunderstanding I've seen a couple of times > > from people. Apart from the general idea of providing buffers, it's > > not that bound to splice. Yes, I reused splicing guts for that > > half-made POC, but we can add a new callback that would do it a > > bit nicer, i.e. better consolidating returned buffers. Would > > ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). > > > probably be even better to have both of them falling back to > > splice so it can cover more cases. The core of it is mediating > > buffers through io_uring's registered buffer table, which > > decouples all the components from each other. > > For using pipe buffer's ->release() to release the whole buffer's > reference, you have to allocate one pipe for each fixed buffer, and add > pipe buffer to it, and keep each pipe buffer into the pipe > until it is consumed, since ->release() needs to be called when > unregistering buffer(all IOs are completed) > > It(allocating/free pipe node, and populating it with each page) is > really inefficient for handling one single IO. > > So re-using splice for this purpose is still bad not mention splice > can't support writeable spliced page. > > Wiring device io buffer with context registered buffer table looks like > another approach, however: > > 1) two uring command OPs for registering/unregistering this buffer in io fast > path has to be added since only userspace can know when buffer(reference) > isn't needed > > 2) userspace becomes more complicated, 3+ OPs are required for handling one > single device IO > > 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, > we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) > for unregistering buffer Follows another problem or complexity here: - normal usage when handling one application(ublk/fuse) IO: register device io buffer (file, buffer key) OP1 consumes the buffer reference and submits IO OP2 consumes the buffer reference and submits IO ... unregister device io buffer(file, buffer key) after all above OPs are completed - for avoiding devil userspace(we are allowed for unprivileged user) to consume buffer after buffer is un-registered, each OP has to grab the buffer(reference)'s reference or check if the buffer is stale in its io code path; which has to be added to current OP code path - so the decoupling purpose may _not_ be supported actually, also the current fixed buffer interface does not support this kind of buffer retrieving via (xarray, (file, key)) thanks, Ming
On 2023/3/19 00:23, Pavel Begunkov wrote: > On 3/16/23 03:13, Xiaoguang Wang wrote: >>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>> and its ->issue() can retrieve/import buffer from master request's >>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>> submits slave OP just like normal OP issued from userspace, that said, >>> SQE order is kept, and batching handling is done too. >> Thanks for this great work, seems that we're now in the right direction >> to support ublk zero copy, I believe this feature will improve io throughput >> greatly and reduce ublk's cpu resource usage. >> >> I have gone through your 2th patch, and have some little concerns here: >> Say we have one ublk loop target device, but it has 4 backend files, >> every file will carry 25% of device capacity and it's implemented in stripped >> way, then for every io request, current implementation will need issed 4 >> fused_cmd, right? 4 slave sqes are necessary, but it would be better to >> have just one master sqe, so I wonder whether we can have another >> method. The key point is to let io_uring support register various kernel >> memory objects, which come from kernel, such as ITER_BVEC or >> ITER_KVEC. so how about below actions: >> 1. add a new infrastructure in io_uring, which will support to register >> various kernel memory objects in it, this new infrastructure could be >> maintained in a xarray structure, every memory objects in it will have >> a unique id. This registration could be done in a ublk uring cmd, io_uring >> offers registration interface. >> 2. then any sqe can use these memory objects freely, so long as it >> passes above unique id in sqe properly. >> Above are just rough ideas, just for your reference. > > It precisely hints on what I proposed a bit earlier, that makes > me not alone thinking that it's a good idea to have a design allowing > 1) multiple ops using a buffer and 2) to limiting it to one single > submission because the userspace might want to preprocess a part > of the data, multiplex it or on the opposite divide. I was mostly > coming from non ublk cases, and one example would be such zc recv, > parsing the app level headers and redirecting the rest of the data > somewhere. > > I haven't got a chance to work on it but will return to it in > a week. The discussion was here: > > https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ > Hi Pavel and all, I think it is a good idea to register some kernel objects(such as bvec) in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). I do not think reusing splice is a good idea because splice should run in io-wq. If we have a big sq depth there may be lots of io-wqs. Then lots of context switch may lower the IO performance especially for small IO size. Here are some rough ideas: (1) design a new OPCODE such as IORING_REGISTER_KOBJ to register kernel objects in io_uring or (2) reuse uring-cmd. We can send uring-cmd to drivers(opcode may be CMD_REGISTER_KBUF) and let drivers call io_uring_provide_kbuf() to register kbuf. io_uring_provide_kbuf() is a new function provided by io_uring for drivers. (3) let the driver call io_uring_provide_kbuf() directly. For ublk, this function is called before io_uring_cmd_done(). Regards, Zhang
On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > Hello, > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > and its ->issue() can retrieve/import buffer from master request's > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > submits slave OP just like normal OP issued from userspace, that said, > SQE order is kept, and batching handling is done too. > > Please see detailed design in commit log of the 2th patch, and one big > point is how to handle buffer ownership. > > With this way, it is easy to support zero copy for ublk/fuse device. > > Basically userspace can specify any sub-buffer of the ublk block request > buffer from the fused command just by setting 'offset/len' > in the slave SQE for running slave OP. This way is flexible to implement > io mapping: mirror, stripped, ... > > The 3th & 4th patches enable fused slave support for the following OPs: > > OP_READ/OP_WRITE > OP_SEND/OP_RECV/OP_SEND_ZC > > The other ublk patches cleans ublk driver and implement fused command > for supporting zero copy. > > Follows userspace code: > > https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 > > All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: > > ublk add -t [loop|nbd|qcow2] -z .... > > Basic fs mount/kernel building and builtin test are done, and also not > observe regression on xfstest test over ublk-loop with zero copy. > > Also add liburing test case for covering fused command based on miniublk > of blktest: > > https://github.com/ming1/liburing/commits/fused_cmd_miniublk > > Performance improvement is obvious on memory bandwidth > related workloads, such as, 1~2X improvement on 64K/512K BS > IO test on loop with ramfs backing file. > > Any comments are welcome! > > V3: > - fix build warning reported by kernel test robot > - drop patch for checking fused flags on existed drivers with > ->uring_command(), which isn't necessary, since we do not do that > when adding new ioctl or uring command > - inline io_init_rq() for core code, so just export io_init_slave_req > - return result of failed slave request unconditionally since REQ_F_CQE_SKIP > will be cleared > - pass xfstest over ublk-loop BTW, I just wrote one ublk zero copy document, which describes technical requirement for this feature, and explains why splice isn't good and how fused command solves it, feel free to refer to it when working on candidate approach. https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy Which will be included in V4. Thanks, Ming
On Sat, Mar 18, 2023 at 04:23:35PM +0000, Pavel Begunkov wrote: > On 3/16/23 03:13, Xiaoguang Wang wrote: > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > > and its ->issue() can retrieve/import buffer from master request's > > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > > submits slave OP just like normal OP issued from userspace, that said, > > > SQE order is kept, and batching handling is done too. > > Thanks for this great work, seems that we're now in the right direction > > to support ublk zero copy, I believe this feature will improve io throughput > > greatly and reduce ublk's cpu resource usage. > > > > I have gone through your 2th patch, and have some little concerns here: > > Say we have one ublk loop target device, but it has 4 backend files, > > every file will carry 25% of device capacity and it's implemented in stripped > > way, then for every io request, current implementation will need issed 4 > > fused_cmd, right? 4 slave sqes are necessary, but it would be better to > > have just one master sqe, so I wonder whether we can have another > > method. The key point is to let io_uring support register various kernel > > memory objects, which come from kernel, such as ITER_BVEC or > > ITER_KVEC. so how about below actions: > > 1. add a new infrastructure in io_uring, which will support to register > > various kernel memory objects in it, this new infrastructure could be > > maintained in a xarray structure, every memory objects in it will have > > a unique id. This registration could be done in a ublk uring cmd, io_uring > > offers registration interface. > > 2. then any sqe can use these memory objects freely, so long as it > > passes above unique id in sqe properly. > > Above are just rough ideas, just for your reference. > > It precisely hints on what I proposed a bit earlier, that makes > me not alone thinking that it's a good idea to have a design allowing > 1) multiple ops using a buffer and 2) to limiting it to one single > submission because the userspace might want to preprocess a part > of the data, multiplex it or on the opposite divide. I was mostly > coming from non ublk cases, and one example would be such zc recv, > parsing the app level headers and redirecting the rest of the data > somewhere. Just get some time to think about zc recv. Firstly I understand the buffer shouldn't be provided from userspace unlike storage, given network recv can happen any time, and NIC driver has to put received data into kernel socket recv buffer first. But if yes for some special recv case, the use case is totally different with ublk, and impossible to share any code with ublk. So here suppose the zc recv means to export socket recv buffer to userspace just like the implementation in lwn doc [1]. [1] https://lwn.net/Articles/752188/ But how does userspace pre-process this kernel buffer? mmap is expensive, and copy won't be one option. Or the data is just simply forwarded to somewhere(special case)? If yes, it can become a bit similar with ublk's case in which the device io buffer needn't to be modified and just simply forwarded to FS or socket in most of cases. Then it could be possible to extend fused for supporting it given the buffer lifetime model is useful for generic zero copy, such as: - send fused command(A) to just register buffer(socket recv buffer) with one empty buffer index, then return the buffer index to userspace via CQE( IORING_CQE_F_MORE), but not complete this fused command(A); but it requires socket FS to implement ->uring_command() for providing recv buffer. - after getting recv SQE, userspace can use the registered buffer to do whatever, but direct access on buffer is one problem, since it is simply pages which have to be mapped for handling from userspace - after userspace handles everything(includes net send over this buffer) on the recv buffer, send another fused command or new OP to ask kernel to release buffer by completing fused command(A). However, one corner case is that this fuse command needs to be completed automatically when io_uring exits since app is dead at that time. It should be easy to extend fused command in above way(slave less) since V4 starts to support normal 64byte SQE, and we have enough uring command flags. But not sure if that is what you need. If not, please explain a bit your exact requirement. > > I haven't got a chance to work on it but will return to it in > a week. The discussion was here: > > https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ Thanks, Ming
On 3/21/23 09:17, Ziyang Zhang wrote: > On 2023/3/19 00:23, Pavel Begunkov wrote: >> On 3/16/23 03:13, Xiaoguang Wang wrote: >>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>>> and its ->issue() can retrieve/import buffer from master request's >>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>>> submits slave OP just like normal OP issued from userspace, that said, >>>> SQE order is kept, and batching handling is done too. >>> Thanks for this great work, seems that we're now in the right direction >>> to support ublk zero copy, I believe this feature will improve io throughput >>> greatly and reduce ublk's cpu resource usage. >>> >>> I have gone through your 2th patch, and have some little concerns here: >>> Say we have one ublk loop target device, but it has 4 backend files, >>> every file will carry 25% of device capacity and it's implemented in stripped >>> way, then for every io request, current implementation will need issed 4 >>> fused_cmd, right? 4 slave sqes are necessary, but it would be better to >>> have just one master sqe, so I wonder whether we can have another >>> method. The key point is to let io_uring support register various kernel >>> memory objects, which come from kernel, such as ITER_BVEC or >>> ITER_KVEC. so how about below actions: >>> 1. add a new infrastructure in io_uring, which will support to register >>> various kernel memory objects in it, this new infrastructure could be >>> maintained in a xarray structure, every memory objects in it will have >>> a unique id. This registration could be done in a ublk uring cmd, io_uring >>> offers registration interface. >>> 2. then any sqe can use these memory objects freely, so long as it >>> passes above unique id in sqe properly. >>> Above are just rough ideas, just for your reference. >> >> It precisely hints on what I proposed a bit earlier, that makes >> me not alone thinking that it's a good idea to have a design allowing >> 1) multiple ops using a buffer and 2) to limiting it to one single >> submission because the userspace might want to preprocess a part >> of the data, multiplex it or on the opposite divide. I was mostly >> coming from non ublk cases, and one example would be such zc recv, >> parsing the app level headers and redirecting the rest of the data >> somewhere. >> >> I haven't got a chance to work on it but will return to it in >> a week. The discussion was here: >> >> https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ >> > > Hi Pavel and all, > > I think it is a good idea to register some kernel objects(such as bvec) > in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. > There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS > and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). > > I do not think reusing splice is a good idea because splice should run in io-wq. The reason why I disabled inline splice execution is because do_splice() and below the stack doesn't support nowait well enough, which is not a problem when we hook directly under the ->splice_read() callback and operate only with one file at a time at the io_uring level. > If we have a big sq depth there may be lots of io-wqs. Then lots of context switch > may lower the IO performance especially for small IO size. > > Here are some rough ideas: > (1) design a new OPCODE such as IORING_REGISTER_KOBJ to register kernel objects in > io_uring or > (2) reuse uring-cmd. We can send uring-cmd to drivers(opcode may be CMD_REGISTER_KBUF) > and let drivers call io_uring_provide_kbuf() to register kbuf. io_uring_provide_kbuf() > is a new function provided by io_uring for drivers. > (3) let the driver call io_uring_provide_kbuf() directly. For ublk, this function is called > before io_uring_cmd_done().
Hi Ziyang, On Tue, Mar 21, 2023 at 05:17:56PM +0800, Ziyang Zhang wrote: > On 2023/3/19 00:23, Pavel Begunkov wrote: > > On 3/16/23 03:13, Xiaoguang Wang wrote: > >>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > >>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > >>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > >>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > >>> and its ->issue() can retrieve/import buffer from master request's > >>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > >>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > >>> submits slave OP just like normal OP issued from userspace, that said, > >>> SQE order is kept, and batching handling is done too. > >> Thanks for this great work, seems that we're now in the right direction > >> to support ublk zero copy, I believe this feature will improve io throughput > >> greatly and reduce ublk's cpu resource usage. > >> > >> I have gone through your 2th patch, and have some little concerns here: > >> Say we have one ublk loop target device, but it has 4 backend files, > >> every file will carry 25% of device capacity and it's implemented in stripped > >> way, then for every io request, current implementation will need issed 4 > >> fused_cmd, right? 4 slave sqes are necessary, but it would be better to > >> have just one master sqe, so I wonder whether we can have another > >> method. The key point is to let io_uring support register various kernel > >> memory objects, which come from kernel, such as ITER_BVEC or > >> ITER_KVEC. so how about below actions: > >> 1. add a new infrastructure in io_uring, which will support to register > >> various kernel memory objects in it, this new infrastructure could be > >> maintained in a xarray structure, every memory objects in it will have > >> a unique id. This registration could be done in a ublk uring cmd, io_uring > >> offers registration interface. > >> 2. then any sqe can use these memory objects freely, so long as it > >> passes above unique id in sqe properly. > >> Above are just rough ideas, just for your reference. > > > > It precisely hints on what I proposed a bit earlier, that makes > > me not alone thinking that it's a good idea to have a design allowing > > 1) multiple ops using a buffer and 2) to limiting it to one single > > submission because the userspace might want to preprocess a part > > of the data, multiplex it or on the opposite divide. I was mostly > > coming from non ublk cases, and one example would be such zc recv, > > parsing the app level headers and redirecting the rest of the data > > somewhere. > > > > I haven't got a chance to work on it but will return to it in > > a week. The discussion was here: > > > > https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ > > > > Hi Pavel and all, > > I think it is a good idea to register some kernel objects(such as bvec) > in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. > There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS > and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). > > I do not think reusing splice is a good idea because splice should run in io-wq. > If we have a big sq depth there may be lots of io-wqs. Then lots of context switch > may lower the IO performance especially for small IO size. Agree, not only it is hard for splice to guarantee correctness of buffer lifetime, but also it is much less efficient to support the feature in one very ugly way, not mention Linus objects to extend splice wrt. buffer direction issue, see the reasoning in my document: https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy > > Here are some rough ideas: > (1) design a new OPCODE such as IORING_REGISTER_KOBJ to register kernel objects in > io_uring or > (2) reuse uring-cmd. We can send uring-cmd to drivers(opcode may be CMD_REGISTER_KBUF) > and let drivers call io_uring_provide_kbuf() to register kbuf. io_uring_provide_kbuf() > is a new function provided by io_uring for drivers. > (3) let the driver call io_uring_provide_kbuf() directly. For ublk, this function is called > before io_uring_cmd_done(). Can you explain a bit which use cases you are trying to address by registering kernel io buffer unmapped to userspace? The buffer(request buffer, represented by bvec) are just bvecs, basically only physical pages available, and the userspace does not have mapping(virtual address) on this buffer and can't read/write the buffer, so I don't think it makes sense to register the buffer somewhere for userspace, does it? That said the buffer should only be used by kernel, such as io_uring normal OPs. It is basically invisible for userspace, However, Xiaoguang's BPF might be one perfect supplement here[1], such as: - add one generic io_uring BPF OP, which can run one specified registered BPF program by passing bpf_prog_id - link this BPF OP as slave request of fused command, then the ebpf prog can do whatever on the kernel pages if kernel mapping & buffer read/write is allowed for ebpf prog, and results can be returned into user via any bpf mapping(s) - then userspace can decide how to handle the result from bpf mapping(s), such as, submit another fused command to handle IO with part of the kernel buffer. Also the buffer is io buffer, and its lifetime is pretty short, and register/ unregister introduces unnecessary cost in fast io path for any approach. Finally it is pretty easy to extend fused command[2] for supporting this kind of interface[2], but at least you need to share your use case first. [1] https://lwn.net/Articles/927356/ [2] https://lore.kernel.org/linux-block/ZBnTuX+5D8QeLPuQ@ovpn-8-18.pek2.redhat.com/T/#m0b8d0dcca5024765cef0439ef1d8ca3f7b38bd1c Thanks, Ming
On Mon, Mar 27, 2023 at 05:04:01PM +0100, Pavel Begunkov wrote: > On 3/21/23 09:17, Ziyang Zhang wrote: > > On 2023/3/19 00:23, Pavel Begunkov wrote: > > > On 3/16/23 03:13, Xiaoguang Wang wrote: > > > > > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > > > > > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > > > > > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > > > > > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > > > > > and its ->issue() can retrieve/import buffer from master request's > > > > > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > > > > > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > > > > > submits slave OP just like normal OP issued from userspace, that said, > > > > > SQE order is kept, and batching handling is done too. > > > > Thanks for this great work, seems that we're now in the right direction > > > > to support ublk zero copy, I believe this feature will improve io throughput > > > > greatly and reduce ublk's cpu resource usage. > > > > > > > > I have gone through your 2th patch, and have some little concerns here: > > > > Say we have one ublk loop target device, but it has 4 backend files, > > > > every file will carry 25% of device capacity and it's implemented in stripped > > > > way, then for every io request, current implementation will need issed 4 > > > > fused_cmd, right? 4 slave sqes are necessary, but it would be better to > > > > have just one master sqe, so I wonder whether we can have another > > > > method. The key point is to let io_uring support register various kernel > > > > memory objects, which come from kernel, such as ITER_BVEC or > > > > ITER_KVEC. so how about below actions: > > > > 1. add a new infrastructure in io_uring, which will support to register > > > > various kernel memory objects in it, this new infrastructure could be > > > > maintained in a xarray structure, every memory objects in it will have > > > > a unique id. This registration could be done in a ublk uring cmd, io_uring > > > > offers registration interface. > > > > 2. then any sqe can use these memory objects freely, so long as it > > > > passes above unique id in sqe properly. > > > > Above are just rough ideas, just for your reference. > > > > > > It precisely hints on what I proposed a bit earlier, that makes > > > me not alone thinking that it's a good idea to have a design allowing > > > 1) multiple ops using a buffer and 2) to limiting it to one single > > > submission because the userspace might want to preprocess a part > > > of the data, multiplex it or on the opposite divide. I was mostly > > > coming from non ublk cases, and one example would be such zc recv, > > > parsing the app level headers and redirecting the rest of the data > > > somewhere. > > > > > > I haven't got a chance to work on it but will return to it in > > > a week. The discussion was here: > > > > > > https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ > > > > > > > Hi Pavel and all, > > > > I think it is a good idea to register some kernel objects(such as bvec) > > in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. > > There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS > > and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). > > > > I do not think reusing splice is a good idea because splice should run in io-wq. > > The reason why I disabled inline splice execution is because do_splice() > and below the stack doesn't support nowait well enough, which is not a > problem when we hook directly under the ->splice_read() callback and > operate only with one file at a time at the io_uring level. I believe I have explained several times[1][2] it isn't good solution for ublk zero copy. But if you insist on reusing splice for this feature, please share your code and I'm happy to give an review. [1] https://lore.kernel.org/linux-block/ZB8B8cr1%2FqIcPdRM@ovpn-8-21.pek2.redhat.com/T/#m1bfa358524b6af94731bcd5be28056f9f4408ecf [2] https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy Thanks, Ming
On 3/18/23 23:42, Ming Lei wrote: > On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: >> On 3/18/23 13:35, Ming Lei wrote: >>> On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: >>>> On 3/17/23 2:14?AM, Ming Lei wrote: >>>>> On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: [...] >>> IMO, splice(->splice_read()) can help much less in this use case, and >>> I can't see improvement David Howells has done in this area: >> >> Let me correct a misunderstanding I've seen a couple of times >> from people. Apart from the general idea of providing buffers, it's >> not that bound to splice. Yes, I reused splicing guts for that >> half-made POC, but we can add a new callback that would do it a >> bit nicer, i.e. better consolidating returned buffers. Would > > ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). >> probably be even better to have both of them falling back to >> splice so it can cover more cases. The core of it is mediating >> buffers through io_uring's registered buffer table, which >> decouples all the components from each other. > > For using pipe buffer's ->release() to release the whole buffer's > reference, you have to allocate one pipe for each fixed buffer, and add > pipe buffer to it, and keep each pipe buffer into the pipe > until it is consumed, since ->release() needs to be called when > unregistering buffer(all IOs are completed) What I'm saying is that I'm more concerned about the uapi, whether internally it's ->splice_read(). I think ->splice_read() has its merit in a hybrid approach, but simplicity let's say for we don't use it and there is a new f_op callback or it's it's returned with by cmd requests. > It(allocating/free pipe node, and populating it with each page) is > really inefficient for handling one single IO. It doesn't need pipe node allocation. We'd need to allocate space for pages, but again, there is a good io_uring infra for it without any single additional lock taken in most cases. > So re-using splice for this purpose is still bad not mention splice > can't support writeable spliced page. > > Wiring device io buffer with context registered buffer table looks like > another approach, however: > > 1) two uring command OPs for registering/unregistering this buffer in io fast > path has to be added since only userspace can know when buffer(reference) > isn't needed Yes, that's a good point. Registration replaces fuse master cmd, so it's one extra request for unregister, which might be fine. > 2) userspace becomes more complicated, 3+ OPs are required for handling one > single device IO > > 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, > we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) > for unregistering buffer It should not necessarily be a file. > 4) here the case is totally different with io_mapped_ubuf which isn't > related to any specific file, and just belong to io_uring context; however, > the device io buffer belongs to device(file) actually, so in theory it is wrong > to put it into context's registered buffer table, and supposed to put into Not at all, it doesn't belong to io_uring but rather to the user space, without a file, right, but io_uring still only borrowing it. As for keeping files, I predict that it'll be there anyway in some time, some p2pdma experiments, dma preregistration, all required having a file attached to the buffer. > per-file buffer table which isn't supported by io_uring, or it becomes hard to > implement multiple-device io buffer in single context since 'file + buffer key' > has to be used to retrieve this buffer, probably xarray has to be > relied, but I was proposing to give slot selection to the userspace, perhaps with optional auto index allocation as it's done with registered files. > - here the index is (file, buffer key) if the table is per-context, current > xarray only supports index with type of 'unsigned long', so looks not doable > - or per-file xarray has to be used, then the implementation becomes more complicated > - write to xarray has to be done two times in fast io path, so another factor which > hurts performance. > >> >>> 1) we need to pass reference of the whole buffer from driver to io_uring, >>> which is missed in splice, which just deals with page reference; for >>> passing whole buffer reference, we have to apply per buffer pipe to >>> solve the problem, and this way is expensive since the pipe can't >>> be freed until all buffers are consumed. >>> >>> 2) reference can't outlive the whole buffer, and splice still misses >>> mechanism to provide such guarantee; splice can just make sure that >>> page won't be gone if page reference is grabbed, but here we care >>> more the whole buffer & its (shared)references lifetime >>> >>> 3) current ->splice_read() misses capability to provide writeable >>> reference to spliced page[2]; either we have to pass new flags >>> to ->splice_read() or passing back new pipe buf flags, unfortunately >>> Linus thought it isn't good to extend pipe/splice for such purpose, >>> and now I agree with Linus now. >> >> It might be a non-workable option if we're thinking about splice(2) >> and pipes, but pipes and ->splice_read() are just internal details, >> an execution mechanism, and it's hidden from the userspace. > > both pipe and ->splice_read() are really exposed to userspace, and are > used in other non-io_uring situations, so any change can not break > existed splice/pipe usage, maybe I misunderstand your point? Oh, I meant reusing some of splice bits but not changing splice(2). E.g. a kernel internal flag which is not allowed to be passed into splice(2). >> I guess someone might make a point that we don't want any changes >> to the splice code even if it doesn't affect splice(2) userspace >> users, but that's rather a part of development process. >>> I believe that Pavel has realized this point[3] too, and here the only >>> of value of using pipe is to reuse ->splice_read(), however, the above >>> points show that ->splice_read() isn't good at this purpose. >> >> But agree that, ->splice_read() doesn't support the revers >> direction, i.e. a file (e.g. ublk) provides buffers for >> someone to write into it, that would need to be extended >> in some way. > > Linus has objected[1] explicitly to extend it in this way: > > There's no point trying to deal with "if unexpectedly doing crazy > things". If a sink writes the data, the sinkm is so unbelievably buggy > that it's not even funny. As far as I can see, Linus doesn't like there that the semantics is not clear. "sink writes data" and writing to pages provided by ->splice_read() don't sound right indeed. I might be wrong but it appears that the semantics was ublk lending an "empty" buffer to another file, which will fill it in and return back the data by calling some sort of ->release callback, then ublk consumes the data. > [1] https://lore.kernel.org/linux-block/CAHk-=wgJsi7t7YYpuo6ewXGnHz2nmj67iWR6KPGoz5TBu34mWQ@mail.gmail.com/ > > That is also the reason why fuse can only support write zero copy via splice > for 10+ years. > >> >>> [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ >> >> Oops, missed this one >> >>> [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ >> >> Miklos said that it's better to signal the owner of buffer about >> completion, IIUC the way I was proposing, i.e. calling ->release >> when io_uring removes the buffer and all io_uring requests using >> it complete, should do exactly that. > > ->release() just for acking the page consumption, what the ublk needs is > to drop the whole buffer(represented by bvec) reference when the buffer isn't > used by normal OPs, actually similar with fuse's case, because buffer > reference can't outlive the buffer itself(repesented by bvec). > > Yeah, probably releasing whole buffer reference can be done by ->release() in > very complicated way, but the whole pipe & pipe buffer has to be kept in > the whole IO lifetime for calling each pipe buffer's ->release(), so you have to > allocate one pipe when registering this buffer, and release it when un-registering > it. Much less efficient. As per noted above, We don't necessarily have to stick with splice_read() and pipe callbacks. > > In short, splice can't help us for meeting ublk/fuse requirement.
On 3/28/23 02:01, Ming Lei wrote: > On Mon, Mar 27, 2023 at 05:04:01PM +0100, Pavel Begunkov wrote: >> On 3/21/23 09:17, Ziyang Zhang wrote: >>> On 2023/3/19 00:23, Pavel Begunkov wrote: >>>> On 3/16/23 03:13, Xiaoguang Wang wrote: >>>>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>>>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>>>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>>>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>>>>> and its ->issue() can retrieve/import buffer from master request's >>>>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>>>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>>>>> submits slave OP just like normal OP issued from userspace, that said, >>>>>> SQE order is kept, and batching handling is done too. >>>>> Thanks for this great work, seems that we're now in the right direction >>>>> to support ublk zero copy, I believe this feature will improve io throughput >>>>> greatly and reduce ublk's cpu resource usage. >>>>> >>>>> I have gone through your 2th patch, and have some little concerns here: >>>>> Say we have one ublk loop target device, but it has 4 backend files, >>>>> every file will carry 25% of device capacity and it's implemented in stripped >>>>> way, then for every io request, current implementation will need issed 4 >>>>> fused_cmd, right? 4 slave sqes are necessary, but it would be better to >>>>> have just one master sqe, so I wonder whether we can have another >>>>> method. The key point is to let io_uring support register various kernel >>>>> memory objects, which come from kernel, such as ITER_BVEC or >>>>> ITER_KVEC. so how about below actions: >>>>> 1. add a new infrastructure in io_uring, which will support to register >>>>> various kernel memory objects in it, this new infrastructure could be >>>>> maintained in a xarray structure, every memory objects in it will have >>>>> a unique id. This registration could be done in a ublk uring cmd, io_uring >>>>> offers registration interface. >>>>> 2. then any sqe can use these memory objects freely, so long as it >>>>> passes above unique id in sqe properly. >>>>> Above are just rough ideas, just for your reference. >>>> >>>> It precisely hints on what I proposed a bit earlier, that makes >>>> me not alone thinking that it's a good idea to have a design allowing >>>> 1) multiple ops using a buffer and 2) to limiting it to one single >>>> submission because the userspace might want to preprocess a part >>>> of the data, multiplex it or on the opposite divide. I was mostly >>>> coming from non ublk cases, and one example would be such zc recv, >>>> parsing the app level headers and redirecting the rest of the data >>>> somewhere. >>>> >>>> I haven't got a chance to work on it but will return to it in >>>> a week. The discussion was here: >>>> >>>> https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ >>>> >>> >>> Hi Pavel and all, >>> >>> I think it is a good idea to register some kernel objects(such as bvec) >>> in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. >>> There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS >>> and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). >>> >>> I do not think reusing splice is a good idea because splice should run in io-wq. >> >> The reason why I disabled inline splice execution is because do_splice() >> and below the stack doesn't support nowait well enough, which is not a >> problem when we hook directly under the ->splice_read() callback and >> operate only with one file at a time at the io_uring level. > > I believe I have explained several times[1][2] it isn't good solution for ublk > zero copy. > > But if you insist on reusing splice for this feature, please share your code and > I'm happy to give an review. Absolutely, I was not available the last week, will be catching up to all that and prototyping it. Let me just note again that my point was not in internally using splice bits but rather in having a different uapi, i.e. mediating with the io_uring's registered buffers. > [1] https://lore.kernel.org/linux-block/ZB8B8cr1%2FqIcPdRM@ovpn-8-21.pek2.redhat.com/T/#m1bfa358524b6af94731bcd5be28056f9f4408ecf > [2] https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy
On Tue, Mar 28, 2023 at 11:55:38AM +0100, Pavel Begunkov wrote: > On 3/18/23 23:42, Ming Lei wrote: > > On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: > > > On 3/18/23 13:35, Ming Lei wrote: > > > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > > > > > On 3/17/23 2:14?AM, Ming Lei wrote: > > > > > > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > [...] > > > > IMO, splice(->splice_read()) can help much less in this use case, and > > > > I can't see improvement David Howells has done in this area: > > > > > > Let me correct a misunderstanding I've seen a couple of times > > > from people. Apart from the general idea of providing buffers, it's > > > not that bound to splice. Yes, I reused splicing guts for that > > > half-made POC, but we can add a new callback that would do it a > > > bit nicer, i.e. better consolidating returned buffers. Would > > > > ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). > > > probably be even better to have both of them falling back to > > > splice so it can cover more cases. The core of it is mediating > > > buffers through io_uring's registered buffer table, which > > > decouples all the components from each other. > > > > For using pipe buffer's ->release() to release the whole buffer's > > reference, you have to allocate one pipe for each fixed buffer, and add > > pipe buffer to it, and keep each pipe buffer into the pipe > > until it is consumed, since ->release() needs to be called when > > unregistering buffer(all IOs are completed) > > What I'm saying is that I'm more concerned about the uapi, > whether internally it's ->splice_read(). I think ->splice_read() > has its merit in a hybrid approach, but simplicity let's say for > we don't use it and there is a new f_op callback or it's > it's returned with by cmd requests. OK, then forget splice if you add new callback, isn't that what this patchset(just reuse ->uring_cmd()) is doing? > > > It(allocating/free pipe node, and populating it with each page) is > > really inefficient for handling one single IO. > > It doesn't need pipe node allocation. We'd need to allocate > space for pages, but again, there is a good io_uring infra > for it without any single additional lock taken in most cases. Then it is same with this patchset. > > > > So re-using splice for this purpose is still bad not mention splice > > can't support writeable spliced page. > > > > Wiring device io buffer with context registered buffer table looks like > > another approach, however: > > > > 1) two uring command OPs for registering/unregistering this buffer in io fast > > path has to be added since only userspace can know when buffer(reference) > > isn't needed > > Yes, that's a good point. Registration replaces fuse master cmd, so it's > one extra request for unregister, which might be fine. Unfortunately I don't think this way is good, the problem is that buffer only has physical pages, and doesn't have userspace mapping, so why bother to export it to userspace? As I replied to Ziyang, the current fused command can be extended to this way easily, but I don't know why we need to use the buffer registration, given userspace can't read/write the buffer, and fused command can cover it just fine. > > > 2) userspace becomes more complicated, 3+ OPs are required for handling one > > single device IO > > > > 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, > > we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) > > for unregistering buffer > > It should not necessarily be a file. At least in ublk's case, from io_uring viewpoint, the buffer is owned by ublk device, so we need the device node or file for releasing the buffer. > > > 4) here the case is totally different with io_mapped_ubuf which isn't > > related to any specific file, and just belong to io_uring context; however, > > the device io buffer belongs to device(file) actually, so in theory it is wrong > > to put it into context's registered buffer table, and supposed to put into > > Not at all, it doesn't belong to io_uring but rather to the user space, > without a file, right, but io_uring still only borrowing it. How can one such buffer be owned by userspace? What if the userspace is killed? If you think userspace can grab the buffer reference, that still needs userspace to release the buffer, but that is unreliable, and io_uring has to cover the buffer cleanup in case of userspace exit abnormally. Because buffer lifetime is crossing multiple OPs if you implement buffer register/unregister OPs. And there isn't such issue for fused command which has same lifetime with the buffer. > > As for keeping files, I predict that it'll be there anyway in some time, > some p2pdma experiments, dma preregistration, all required having a file > attached to the buffer. > > > per-file buffer table which isn't supported by io_uring, or it becomes hard to > > implement multiple-device io buffer in single context since 'file + buffer key' > > has to be used to retrieve this buffer, probably xarray has to be > > relied, but > > I was proposing to give slot selection to the userspace, perhaps with > optional auto index allocation as it's done with registered files. As I mentioned above, it doesn't make sense to export buffer to userspace which can't touch any data of the buffer at all. > > > - here the index is (file, buffer key) if the table is per-context, current > > xarray only supports index with type of 'unsigned long', so looks not doable > > - or per-file xarray has to be used, then the implementation becomes more complicated > > - write to xarray has to be done two times in fast io path, so another factor which > > hurts performance. > > > > > > > > > 1) we need to pass reference of the whole buffer from driver to io_uring, > > > > which is missed in splice, which just deals with page reference; for > > > > passing whole buffer reference, we have to apply per buffer pipe to > > > > solve the problem, and this way is expensive since the pipe can't > > > > be freed until all buffers are consumed. > > > > > > > > 2) reference can't outlive the whole buffer, and splice still misses > > > > mechanism to provide such guarantee; splice can just make sure that > > > > page won't be gone if page reference is grabbed, but here we care > > > > more the whole buffer & its (shared)references lifetime > > > > > > > > 3) current ->splice_read() misses capability to provide writeable > > > > reference to spliced page[2]; either we have to pass new flags > > > > to ->splice_read() or passing back new pipe buf flags, unfortunately > > > > Linus thought it isn't good to extend pipe/splice for such purpose, > > > > and now I agree with Linus now. > > > > > > It might be a non-workable option if we're thinking about splice(2) > > > and pipes, but pipes and ->splice_read() are just internal details, > > > an execution mechanism, and it's hidden from the userspace. > > > > both pipe and ->splice_read() are really exposed to userspace, and are > > used in other non-io_uring situations, so any change can not break > > existed splice/pipe usage, maybe I misunderstand your point? > > Oh, I meant reusing some of splice bits but not changing splice(2). > E.g. a kernel internal flag which is not allowed to be passed into > splice(2). > > > > > I guess someone might make a point that we don't want any changes > > > to the splice code even if it doesn't affect splice(2) userspace > > > users, but that's rather a part of development process. > > > > I believe that Pavel has realized this point[3] too, and here the only > > > > of value of using pipe is to reuse ->splice_read(), however, the above > > > > points show that ->splice_read() isn't good at this purpose. > > > > > > But agree that, ->splice_read() doesn't support the revers > > > direction, i.e. a file (e.g. ublk) provides buffers for > > > someone to write into it, that would need to be extended > > > in some way. > > > > Linus has objected[1] explicitly to extend it in this way: > > > > There's no point trying to deal with "if unexpectedly doing crazy > > things". If a sink writes the data, the sinkm is so unbelievably buggy > > that it's not even funny. > > As far as I can see, Linus doesn't like there that the semantics > is not clear. "sink writes data" and writing to pages provided > by ->splice_read() don't sound right indeed. > > I might be wrong but it appears that the semantics was ublk > lending an "empty" buffer to another file, which will fill it > in and return back the data by calling some sort of ->release > callback, then ublk consumes the data. Yes, that is exactly what fused command is doing. > > > > [1] https://lore.kernel.org/linux-block/CAHk-=wgJsi7t7YYpuo6ewXGnHz2nmj67iWR6KPGoz5TBu34mWQ@mail.gmail.com/ > > > > That is also the reason why fuse can only support write zero copy via splice > > for 10+ years. > > > > > > > > > [1] https://lore.kernel.org/linux-block/ZAk5%2FHfwc+NBwlbI@ovpn-8-17.pek2.redhat.com/ > > > > > > Oops, missed this one > > > > > > > [2] https://lore.kernel.org/linux-block/CAJfpeguQ3xn2-6svkkVXJ88tiVfcDd-eKi1evzzfvu305fMoyw@mail.gmail.com/ > > > > > > Miklos said that it's better to signal the owner of buffer about > > > completion, IIUC the way I was proposing, i.e. calling ->release > > > when io_uring removes the buffer and all io_uring requests using > > > it complete, should do exactly that. > > > > ->release() just for acking the page consumption, what the ublk needs is > > to drop the whole buffer(represented by bvec) reference when the buffer isn't > > used by normal OPs, actually similar with fuse's case, because buffer > > reference can't outlive the buffer itself(repesented by bvec). > > > > Yeah, probably releasing whole buffer reference can be done by ->release() in > > very complicated way, but the whole pipe & pipe buffer has to be kept in > > the whole IO lifetime for calling each pipe buffer's ->release(), so you have to > > allocate one pipe when registering this buffer, and release it when un-registering > > it. Much less efficient. > > As per noted above, We don't necessarily have to stick with splice_read() > and pipe callbacks. As I mentioned, it is basically what fused command is doing. Thanks, Ming
On 2023/3/28 08:53, Ming Lei wrote: > Hi Ziyang, > > On Tue, Mar 21, 2023 at 05:17:56PM +0800, Ziyang Zhang wrote: >> On 2023/3/19 00:23, Pavel Begunkov wrote: >>> On 3/16/23 03:13, Xiaoguang Wang wrote: >>>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to >>>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd >>>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs >>>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, >>>>> and its ->issue() can retrieve/import buffer from master request's >>>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of >>>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset >>>>> submits slave OP just like normal OP issued from userspace, that said, >>>>> SQE order is kept, and batching handling is done too. >>>> Thanks for this great work, seems that we're now in the right direction >>>> to support ublk zero copy, I believe this feature will improve io throughput >>>> greatly and reduce ublk's cpu resource usage. >>>> >>>> I have gone through your 2th patch, and have some little concerns here: >>>> Say we have one ublk loop target device, but it has 4 backend files, >>>> every file will carry 25% of device capacity and it's implemented in stripped >>>> way, then for every io request, current implementation will need issed 4 >>>> fused_cmd, right? 4 slave sqes are necessary, but it would be better to >>>> have just one master sqe, so I wonder whether we can have another >>>> method. The key point is to let io_uring support register various kernel >>>> memory objects, which come from kernel, such as ITER_BVEC or >>>> ITER_KVEC. so how about below actions: >>>> 1. add a new infrastructure in io_uring, which will support to register >>>> various kernel memory objects in it, this new infrastructure could be >>>> maintained in a xarray structure, every memory objects in it will have >>>> a unique id. This registration could be done in a ublk uring cmd, io_uring >>>> offers registration interface. >>>> 2. then any sqe can use these memory objects freely, so long as it >>>> passes above unique id in sqe properly. >>>> Above are just rough ideas, just for your reference. >>> >>> It precisely hints on what I proposed a bit earlier, that makes >>> me not alone thinking that it's a good idea to have a design allowing >>> 1) multiple ops using a buffer and 2) to limiting it to one single >>> submission because the userspace might want to preprocess a part >>> of the data, multiplex it or on the opposite divide. I was mostly >>> coming from non ublk cases, and one example would be such zc recv, >>> parsing the app level headers and redirecting the rest of the data >>> somewhere. >>> >>> I haven't got a chance to work on it but will return to it in >>> a week. The discussion was here: >>> >>> https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ >>> >> >> Hi Pavel and all, >> >> I think it is a good idea to register some kernel objects(such as bvec) >> in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. >> There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS >> and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). >> >> I do not think reusing splice is a good idea because splice should run in io-wq. >> If we have a big sq depth there may be lots of io-wqs. Then lots of context switch >> may lower the IO performance especially for small IO size. > > Agree, not only it is hard for splice to guarantee correctness of buffer lifetime, > but also it is much less efficient to support the feature in one very ugly way, not > mention Linus objects to extend splice wrt. buffer direction issue, see the reasoning > in my document: > > https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy > >> >> Here are some rough ideas: >> (1) design a new OPCODE such as IORING_REGISTER_KOBJ to register kernel objects in >> io_uring or >> (2) reuse uring-cmd. We can send uring-cmd to drivers(opcode may be CMD_REGISTER_KBUF) >> and let drivers call io_uring_provide_kbuf() to register kbuf. io_uring_provide_kbuf() >> is a new function provided by io_uring for drivers. >> (3) let the driver call io_uring_provide_kbuf() directly. For ublk, this function is called >> before io_uring_cmd_done(). > > Can you explain a bit which use cases you are trying to address by > registering kernel io buffer unmapped to userspace? Hi Ming, Sorry there is no specific use case. In our product, we have to calculate cksum or compress data before sending IO to remote backend. So Xiaoguang's EBPF might be the final solution... :) But I'd rather to start here... I think you, Pavel and I all have the same basic idea: register the kernel object (bvec) first then incoming sqes can use it. But I think fused-cmd is too specific (hack) to ublk so other users of io_uring may not benefit from it. What if we design a general way which allows io_uring to register kernel objects (such as bvec) just like IORING_OP_PROVIDE_BUFFERS or IORING_REGISTER_PBUF_RING? Pavel said that registration replaces fuse master cmd. And I think so too. > > The buffer(request buffer, represented by bvec) are just bvecs, basically only > physical pages available, and the userspace does not have mapping(virtual address) > on this buffer and can't read/write the buffer, so I don't think it makes sense > to register the buffer somewhere for userspace, does it? The userspace does not touch these registered kernel bvecs, but reference it id. For example, we can set "sqe->kobj_id" so this sqe can import this bvec as its RW buffer just like IORING_OP_PROVIDE_BUFFERS. There is limitation on fused-cmd: secondary sqe has to be primary+1 or be linked. But with registration way we allow multiple OPs reference the kernel bvecs. However we have to deal with buffer ownership/lifetime carefully. > > That said the buffer should only be used by kernel, such as io_uring normal OPs. > It is basically invisible for userspace, > > However, Xiaoguang's BPF might be one perfect supplement here[1], such as: > > - add one generic io_uring BPF OP, which can run one specified registered BPF > program by passing bpf_prog_id > > - link this BPF OP as slave request of fused command, then the ebpf prog can do > whatever on the kernel pages if kernel mapping & buffer read/write is allowed > for ebpf prog, and results can be returned into user via any bpf mapping(s) In Xiaoguang's ublk-EBPF design, we almost avoid userspace code/logic while handling ublk io. So mix fused-cmd with ublk-EBPF may be a bad idea. > > - then userspace can decide how to handle the result from bpf mapping(s), such as, > submit another fused command to handle IO with part of the kernel buffer. > > Also the buffer is io buffer, and its lifetime is pretty short, and register/ > unregister introduces unnecessary cost in fast io path for any approach. I'm not sure the io buffer has short lifetime in our product. :P In our product we can first issue a very big request with a big io buffer. Then the backend can parse&split it into pieces and distribute each piece to a specific socket_fd representing a storage node. This big io buffer may have long lifetime. Regards, Zhang
On 2023/3/28 21:01, Ming Lei wrote: [...] >> >> >>> So re-using splice for this purpose is still bad not mention splice >>> can't support writeable spliced page. >>> >>> Wiring device io buffer with context registered buffer table looks like >>> another approach, however: >>> >>> 1) two uring command OPs for registering/unregistering this buffer in io fast >>> path has to be added since only userspace can know when buffer(reference) >>> isn't needed >> >> Yes, that's a good point. Registration replaces fuse master cmd, so it's >> one extra request for unregister, which might be fine. > > Unfortunately I don't think this way is good, the problem is that buffer > only has physical pages, and doesn't have userspace mapping, so why bother > to export it to userspace? > > As I replied to Ziyang, the current fused command can be extended to > this way easily, but I don't know why we need to use the buffer registration, > given userspace can't read/write the buffer, and fused command can cover > it just fine. > Hi Ming, I have replied to you in another email.
On Wed, Mar 29, 2023 at 02:57:38PM +0800, Ziyang Zhang wrote: > On 2023/3/28 08:53, Ming Lei wrote: > > Hi Ziyang, > > > > On Tue, Mar 21, 2023 at 05:17:56PM +0800, Ziyang Zhang wrote: > >> On 2023/3/19 00:23, Pavel Begunkov wrote: > >>> On 3/16/23 03:13, Xiaoguang Wang wrote: > >>>>> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to > >>>>> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd > >>>>> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs > >>>>> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, > >>>>> and its ->issue() can retrieve/import buffer from master request's > >>>>> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of > >>>>> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset > >>>>> submits slave OP just like normal OP issued from userspace, that said, > >>>>> SQE order is kept, and batching handling is done too. > >>>> Thanks for this great work, seems that we're now in the right direction > >>>> to support ublk zero copy, I believe this feature will improve io throughput > >>>> greatly and reduce ublk's cpu resource usage. > >>>> > >>>> I have gone through your 2th patch, and have some little concerns here: > >>>> Say we have one ublk loop target device, but it has 4 backend files, > >>>> every file will carry 25% of device capacity and it's implemented in stripped > >>>> way, then for every io request, current implementation will need issed 4 > >>>> fused_cmd, right? 4 slave sqes are necessary, but it would be better to > >>>> have just one master sqe, so I wonder whether we can have another > >>>> method. The key point is to let io_uring support register various kernel > >>>> memory objects, which come from kernel, such as ITER_BVEC or > >>>> ITER_KVEC. so how about below actions: > >>>> 1. add a new infrastructure in io_uring, which will support to register > >>>> various kernel memory objects in it, this new infrastructure could be > >>>> maintained in a xarray structure, every memory objects in it will have > >>>> a unique id. This registration could be done in a ublk uring cmd, io_uring > >>>> offers registration interface. > >>>> 2. then any sqe can use these memory objects freely, so long as it > >>>> passes above unique id in sqe properly. > >>>> Above are just rough ideas, just for your reference. > >>> > >>> It precisely hints on what I proposed a bit earlier, that makes > >>> me not alone thinking that it's a good idea to have a design allowing > >>> 1) multiple ops using a buffer and 2) to limiting it to one single > >>> submission because the userspace might want to preprocess a part > >>> of the data, multiplex it or on the opposite divide. I was mostly > >>> coming from non ublk cases, and one example would be such zc recv, > >>> parsing the app level headers and redirecting the rest of the data > >>> somewhere. > >>> > >>> I haven't got a chance to work on it but will return to it in > >>> a week. The discussion was here: > >>> > >>> https://lore.kernel.org/all/ce96f7e7-1315-7154-f540-1a3ff0215674@gmail.com/ > >>> > >> > >> Hi Pavel and all, > >> > >> I think it is a good idea to register some kernel objects(such as bvec) > >> in io_uring and return a cookie(such as buf_idx) for READ/WRITE/SEND/RECV sqes. > >> There are some ways to register user's buffer such as IORING_OP_PROVIDE_BUFFERS > >> and IORING_REGISTER_PBUF_RING but there is not a way to register kernel buffer(bvec). > >> > >> I do not think reusing splice is a good idea because splice should run in io-wq. > >> If we have a big sq depth there may be lots of io-wqs. Then lots of context switch > >> may lower the IO performance especially for small IO size. > > > > Agree, not only it is hard for splice to guarantee correctness of buffer lifetime, > > but also it is much less efficient to support the feature in one very ugly way, not > > mention Linus objects to extend splice wrt. buffer direction issue, see the reasoning > > in my document: > > > > https://github.com/ming1/linux/blob/my_v6.3-io_uring_fuse_cmd_v4/Documentation/block/ublk.rst#zero-copy > > > >> > >> Here are some rough ideas: > >> (1) design a new OPCODE such as IORING_REGISTER_KOBJ to register kernel objects in > >> io_uring or > >> (2) reuse uring-cmd. We can send uring-cmd to drivers(opcode may be CMD_REGISTER_KBUF) > >> and let drivers call io_uring_provide_kbuf() to register kbuf. io_uring_provide_kbuf() > >> is a new function provided by io_uring for drivers. > >> (3) let the driver call io_uring_provide_kbuf() directly. For ublk, this function is called > >> before io_uring_cmd_done(). > > > > Can you explain a bit which use cases you are trying to address by > > registering kernel io buffer unmapped to userspace? > > Hi Ming, > > Sorry there is no specific use case. In our product, we have to calculate cksum > or compress data before sending IO to remote backend. So Xiaoguang's EBPF might > be the final solution... :) But I'd rather to start here... If chsum calculation and compression are done in userspace, the current zero copy can't help you because the fused command is for sharing ublk client io buffer to io_uring OPs only. And userspace has to reply on data copy for checksum & compression. ebpf could help you, but that is still one big project, not sure if current prog is allowed to get kernel mapping of pages and read/write via the kernel mapping. > > I think you, Pavel and I all have the same basic idea: register the kernel object > (bvec) first then incoming sqes can use it. But I think fused-cmd is too specific > (hack) to ublk so other users of io_uring may not benefit from it. fused command is actually one generic interface: 1) create relationship between primary command and secondary requests, the current interface does support to setup 1:N relationship, and just needs multiple secondary reqs following the primary command. If you think following SQEs isn't flexible, you still can send multiple fused requests with same primary cmd to relax the usage of following SQEs. 2) based on the above relationship, lots of thing can be done, sharing buffer is just one function, it could be other kind of resource sharing. The 'sharing' can be implemented as plugin way, such as passing uring_command flags for specifying which kind of plugin is used. I have re-organized code in my local repo in the above way. > What if we design a general way which allows io_uring to register kernel objects > (such as bvec) just like IORING_OP_PROVIDE_BUFFERS or IORING_REGISTER_PBUF_RING? > Pavel said that registration replaces fuse master cmd. And I think so too. The buffer belongs to device, not io_uring context. And the registration isn't necessary, and not sure it is doable: 1) userspace hasn't buffer mapping, so can't use the buffer, you can't calculate checksum and compress data by this registration 2) you just want to use the register id to build the relationship between primary command and secondary OPs, but fused command can do it(see above) because we want to solve buffer lifetime easily, fused command has same lifetime with the buffer reference 3) not sure if the buffer registration is doable: - only 1 sqe flags is left, how to distinguish normal fixed buffer with this kind of registration? - the buffer belongs to device, if you register it in userspace, you have to unregister it in userspace since only userspace knows when the buffer isn't needed. Then this buffer lifetime will cross multiple OPs, what if the userspace is killed before unregistration. So what is your real requirement for the buffer registration? I believe fused command can solve requests relationship building(primary cmd vs. secondary requests), which seems your only concern about buffer registration. > > > > > The buffer(request buffer, represented by bvec) are just bvecs, basically only > > physical pages available, and the userspace does not have mapping(virtual address) > > on this buffer and can't read/write the buffer, so I don't think it makes sense > > to register the buffer somewhere for userspace, does it? > > The userspace does not touch these registered kernel bvecs, but reference it id. > For example, we can set "sqe->kobj_id" so this sqe can import this bvec as its > RW buffer just like IORING_OP_PROVIDE_BUFFERS. > > There is limitation on fused-cmd: secondary sqe has to be primary+1 or be linked. > But with registration way we allow multiple OPs reference the kernel bvecs. The interface in V5 actually starts to supports to 1:N relation between primary cmd and secondary requests, but just implements 1:1 so far. It isn't hard to do 1:N. Actually you can reach same purpose by sending multiple fused requests with same primary req, and there shouldn't be performance effect since the primary command handling is pretty thin(passing buffer reference). > However > we have to deal with buffer ownership/lifetime carefully. That is one fundamental problem. If buffer is allowed to cross multiple OPs, it can be hard to solve the lifetime issue. Not mention it is less efficient to add one extra buffer un-registraion in fast io path. > > > > > That said the buffer should only be used by kernel, such as io_uring normal OPs. > > It is basically invisible for userspace, > > > > However, Xiaoguang's BPF might be one perfect supplement here[1], such as: > > > > - add one generic io_uring BPF OP, which can run one specified registered BPF > > program by passing bpf_prog_id > > > > - link this BPF OP as slave request of fused command, then the ebpf prog can do > > whatever on the kernel pages if kernel mapping & buffer read/write is allowed > > for ebpf prog, and results can be returned into user via any bpf mapping(s) > > In Xiaoguang's ublk-EBPF design, we almost avoid userspace code/logic while > handling ublk io. So mix fused-cmd with ublk-EBPF may be a bad idea. What I meant is to add io_uring generic ebpf OP, that isn't ublk dedicated ebpf. The generic io_uring ebpf OP is for supporting encryption, checksum, or simple packet parsing, sort of thing, because the bvec buffer doesn't have userspace mapping, and we want to avoid to copy data to userspace for calculating checksum, encryption, ... > > > > > - then userspace can decide how to handle the result from bpf mapping(s), such as, > > submit another fused command to handle IO with part of the kernel buffer. > > > > Also the buffer is io buffer, and its lifetime is pretty short, and register/ > > unregister introduces unnecessary cost in fast io path for any approach. > > I'm not sure the io buffer has short lifetime in our product. :P In our product > we can first issue a very big request with a big io buffer. Then the backend > can parse&split it into pieces and distribute each piece to a specific socket_fd > representing a storage node. This big io buffer may have long lifetime. The short just means it is in fast io path, not like io_uring fixed buffer which needs to register just once. IO handling is really fast, otherwise it isn't necessary to consider zero copy at all. So we do care performance effect from any unneccessary operation(such as, buffer unregistration). Thanks, Ming
On 3/28/23 14:01, Ming Lei wrote: > On Tue, Mar 28, 2023 at 11:55:38AM +0100, Pavel Begunkov wrote: >> On 3/18/23 23:42, Ming Lei wrote: >>> On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: >>>> On 3/18/23 13:35, Ming Lei wrote: >>>>> On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: >>>>>> On 3/17/23 2:14?AM, Ming Lei wrote: >>>>>>> On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: >> [...] >>>>> IMO, splice(->splice_read()) can help much less in this use case, and >>>>> I can't see improvement David Howells has done in this area: >>>> >>>> Let me correct a misunderstanding I've seen a couple of times >>>> from people. Apart from the general idea of providing buffers, it's >>>> not that bound to splice. Yes, I reused splicing guts for that >>>> half-made POC, but we can add a new callback that would do it a >>>> bit nicer, i.e. better consolidating returned buffers. Would >>> >>> ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). >>>> probably be even better to have both of them falling back to >>>> splice so it can cover more cases. The core of it is mediating >>>> buffers through io_uring's registered buffer table, which >>>> decouples all the components from each other. >>> >>> For using pipe buffer's ->release() to release the whole buffer's >>> reference, you have to allocate one pipe for each fixed buffer, and add >>> pipe buffer to it, and keep each pipe buffer into the pipe >>> until it is consumed, since ->release() needs to be called when >>> unregistering buffer(all IOs are completed) >> >> What I'm saying is that I'm more concerned about the uapi, >> whether internally it's ->splice_read(). I think ->splice_read() >> has its merit in a hybrid approach, but simplicity let's say for >> we don't use it and there is a new f_op callback or it's >> it's returned with by cmd requests. > > OK, then forget splice if you add new callback, isn't that what this > patchset(just reuse ->uring_cmd()) is doing? It certainly similar in many aspects! And it's also similar to splicing with pipes, just instead of pipes there is io_uring and, of course, semantics changes. The idea is to decouple requests from each other with a different uapi. >>> It(allocating/free pipe node, and populating it with each page) is >>> really inefficient for handling one single IO. >> >> It doesn't need pipe node allocation. We'd need to allocate >> space for pages, but again, there is a good io_uring infra >> for it without any single additional lock taken in most cases. > > Then it is same with this patchset. > >> >> >>> So re-using splice for this purpose is still bad not mention splice >>> can't support writeable spliced page. >>> >>> Wiring device io buffer with context registered buffer table looks like >>> another approach, however: >>> >>> 1) two uring command OPs for registering/unregistering this buffer in io fast >>> path has to be added since only userspace can know when buffer(reference) >>> isn't needed >> >> Yes, that's a good point. Registration replaces fuse master cmd, so it's >> one extra request for unregister, which might be fine. > > Unfortunately I don't think this way is good, the problem is that buffer > only has physical pages, and doesn't have userspace mapping, so why bother > to export it to userspace? > > As I replied to Ziyang, the current fused command can be extended to > this way easily, but I don't know why we need to use the buffer registration, > given userspace can't read/write the buffer, and fused command can cover > it just fine. I probably mentioned it before, but that's where we need a new memcpy io_uring request type, to partially copy it, e.g. headers. I think people mentioned memcpy before in general, and it will also be used for DMA driven copies if Keith returns back to experiments. Apart from it and things like broadcasting, sending different chunks to different places and so, there is a typical problem what to do when the second operation fails but the data has already been received, mostly relevant to sockets / streams. >>> 2) userspace becomes more complicated, 3+ OPs are required for handling one >>> single device IO >>> >>> 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, >>> we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) >>> for unregistering buffer >> >> It should not necessarily be a file. > > At least in ublk's case, from io_uring viewpoint, the buffer is owned by > ublk device, so we need the device node or file for releasing the > buffer. For example, io_uring has a lightweight way to pin the context (pcpu refcount). I haven't looked into ublk code, it's hard for me to argue about it. >>> 4) here the case is totally different with io_mapped_ubuf which isn't >>> related to any specific file, and just belong to io_uring context; however, >>> the device io buffer belongs to device(file) actually, so in theory it is wrong >>> to put it into context's registered buffer table, and supposed to put into >> >> Not at all, it doesn't belong to io_uring but rather to the user space, >> without a file, right, but io_uring still only borrowing it. > > How can one such buffer be owned by userspace? What if the userspace is > killed? If you think userspace can grab the buffer reference, that still > needs userspace to release the buffer, but that is unreliable, and > io_uring has to cover the buffer cleanup in case of userspace exit abnormally. Conceptually userspace owns buffers and io_uring is share / borrowing it. Probably, I misunderstood and you was talking about refcounting or something else. Can you elaborate? As for references, io_uring pins normal buffers and so holds additional refs. > Because buffer lifetime is crossing multiple OPs if you implement buffer > register/unregister OPs. And there isn't such issue for fused command > which has same lifetime with the buffer. > >> >> As for keeping files, I predict that it'll be there anyway in some time, >> some p2pdma experiments, dma preregistration, all required having a file >> attached to the buffer. >> >>> per-file buffer table which isn't supported by io_uring, or it becomes hard to >>> implement multiple-device io buffer in single context since 'file + buffer key' >>> has to be used to retrieve this buffer, probably xarray has to be >>> relied, but >> >> I was proposing to give slot selection to the userspace, perhaps with >> optional auto index allocation as it's done with registered files. > > As I mentioned above, it doesn't make sense to export buffer to > userspace which can't touch any data of the buffer at all. replied above. >>> - here the index is (file, buffer key) if the table is per-context, current >>> xarray only supports index with type of 'unsigned long', so looks not doable >>> - or per-file xarray has to be used, then the implementation becomes more complicated >>> - write to xarray has to be done two times in fast io path, so another factor which >>> hurts performance. >>> >>>> [...]
On Wed, Mar 29, 2023 at 11:43:41AM +0100, Pavel Begunkov wrote: > On 3/28/23 14:01, Ming Lei wrote: > > On Tue, Mar 28, 2023 at 11:55:38AM +0100, Pavel Begunkov wrote: > > > On 3/18/23 23:42, Ming Lei wrote: > > > > On Sat, Mar 18, 2023 at 04:51:14PM +0000, Pavel Begunkov wrote: > > > > > On 3/18/23 13:35, Ming Lei wrote: > > > > > > On Sat, Mar 18, 2023 at 06:59:41AM -0600, Jens Axboe wrote: > > > > > > > On 3/17/23 2:14?AM, Ming Lei wrote: > > > > > > > > On Tue, Mar 14, 2023 at 08:57:11PM +0800, Ming Lei wrote: > > > [...] > > > > > > IMO, splice(->splice_read()) can help much less in this use case, and > > > > > > I can't see improvement David Howells has done in this area: > > > > > > > > > > Let me correct a misunderstanding I've seen a couple of times > > > > > from people. Apart from the general idea of providing buffers, it's > > > > > not that bound to splice. Yes, I reused splicing guts for that > > > > > half-made POC, but we can add a new callback that would do it a > > > > > bit nicer, i.e. better consolidating returned buffers. Would > > > > > > > > ->release() is for releasing pipe-buffer(page), instead of the whole buffer(reference). > > > > > probably be even better to have both of them falling back to > > > > > splice so it can cover more cases. The core of it is mediating > > > > > buffers through io_uring's registered buffer table, which > > > > > decouples all the components from each other. > > > > > > > > For using pipe buffer's ->release() to release the whole buffer's > > > > reference, you have to allocate one pipe for each fixed buffer, and add > > > > pipe buffer to it, and keep each pipe buffer into the pipe > > > > until it is consumed, since ->release() needs to be called when > > > > unregistering buffer(all IOs are completed) > > > > > > What I'm saying is that I'm more concerned about the uapi, > > > whether internally it's ->splice_read(). I think ->splice_read() > > > has its merit in a hybrid approach, but simplicity let's say for > > > we don't use it and there is a new f_op callback or it's > > > it's returned with by cmd requests. > > > > OK, then forget splice if you add new callback, isn't that what this > > patchset(just reuse ->uring_cmd()) is doing? > > It certainly similar in many aspects! And it's also similar to > splicing with pipes, just instead of pipes there is io_uring and, It is definitely different with pipe/splice, which works on page lifetime, but here we need to focus on the whole buffer lifetime. > of course, semantics changes. The idea is to decouple requests from > each other with a different uapi. The only difference is that buffer registration can use current ->buffer_idx interface, and fused command uses normal uapi interface by passing buffer offset/len via sqe->addr & sqe->len to locate buffer in primary command. That should be the decouple. But not sure if the difference matters. Even though I am not sure if it is doable, because: - only 1 sqe flag is left, and how to differentiate this buffer registration with normal fixed buffer - unregister buffer OP may not be called because of task exit abnormally, so io_uring has to take care of the cleanup, so file/command data needs to be saved somewhere for the cleanup, since buffer belongs to device, both register and unregister should call into device via uring command, see details below Also there are other performance effects from buffer registration: 1) one extra OP of unregister is needed in io code path 2) boundary in buffer register & OPs & buffer unregister have to be linked since there are dependencies among the three(register, OPs, unregister) > > > > > It(allocating/free pipe node, and populating it with each page) is > > > > really inefficient for handling one single IO. > > > > > > It doesn't need pipe node allocation. We'd need to allocate > > > space for pages, but again, there is a good io_uring infra > > > for it without any single additional lock taken in most cases. > > > > Then it is same with this patchset. > > > > > > > > > > > > So re-using splice for this purpose is still bad not mention splice > > > > can't support writeable spliced page. > > > > > > > > Wiring device io buffer with context registered buffer table looks like > > > > another approach, however: > > > > > > > > 1) two uring command OPs for registering/unregistering this buffer in io fast > > > > path has to be added since only userspace can know when buffer(reference) > > > > isn't needed > > > > > > Yes, that's a good point. Registration replaces fuse master cmd, so it's > > > one extra request for unregister, which might be fine. > > > > Unfortunately I don't think this way is good, the problem is that buffer > > only has physical pages, and doesn't have userspace mapping, so why bother > > to export it to userspace? > > > > As I replied to Ziyang, the current fused command can be extended to > > this way easily, but I don't know why we need to use the buffer registration, > > given userspace can't read/write the buffer, and fused command can cover > > it just fine. > > I probably mentioned it before, but that's where we need a new memcpy > io_uring request type, to partially copy it, e.g. headers. I think people > mentioned memcpy before in general, and it will also be used for DMA driven > copies if Keith returns back to experiments. > > Apart from it and things like broadcasting, sending different chunks to > different places and so, there is a typical problem what to do when the > second operation fails but the data has already been received, mostly > relevant to sockets / streams. OK, but the new copy OP can work with both fused command and buffer registration if it is involved, and buffer register isn't a must given the interface needs to support plain offset/len way. > > > > > 2) userspace becomes more complicated, 3+ OPs are required for handling one > > > > single device IO > > > > > > > > 3) buffer reference crosses multiple OPs, for cleanup the registered buffer, > > > > we have to store the device file & "buffer key" in each buffer(such as io_uring_bvec_buf) > > > > for unregistering buffer > > > > > > It should not necessarily be a file. > > > > At least in ublk's case, from io_uring viewpoint, the buffer is owned by > > ublk device, so we need the device node or file for releasing the > > buffer. > > For example, io_uring has a lightweight way to pin the context > (pcpu refcount). I haven't looked into ublk code, it's hard for > me to argue about it. The buffer(generic bio/bvec pages) is originated from generic application which submits IO to /dev/ublkbN(block device), or page cache, and io_uring borrows the buffer via uring command on /dev/ublkbcN(pair device of /dev/ublkbN). > > > > > 4) here the case is totally different with io_mapped_ubuf which isn't > > > > related to any specific file, and just belong to io_uring context; however, > > > > the device io buffer belongs to device(file) actually, so in theory it is wrong > > > > to put it into context's registered buffer table, and supposed to put into > > > > > > Not at all, it doesn't belong to io_uring but rather to the user space, > > > without a file, right, but io_uring still only borrowing it. > > > > How can one such buffer be owned by userspace? What if the userspace is > > killed? If you think userspace can grab the buffer reference, that still > > needs userspace to release the buffer, but that is unreliable, and > > io_uring has to cover the buffer cleanup in case of userspace exit abnormally. > > Conceptually userspace owns buffers and io_uring is share / borrowing it. > Probably, I misunderstood and you was talking about refcounting or something > else. Can you elaborate? As for references, io_uring pins normal buffers > and so holds additional refs. We need one uring command on /dev/ublkcN to get the buffer, and the buffer needs to be return back after we run OPs with the buffer. Follows difference between the two approaches: 1) fused command - the buffer lifetime is same with the primary command which is completed after all secondary OPs are completed with the buffer, so driver gets notified after we use the buffer which belongs to /dev/ublkcN 2) buffer registration - one uring command to get the buffer from /dev/ublkcN and register it into io_uring - submit OPs with the buffer - unregister the buffer after all above OPs are done, which still needs one uring command on /dev/ulkbcN That is why I mentioned it is hard to handle buffer cleanup after userspace exits abnormally with buffer registration. > > > Because buffer lifetime is crossing multiple OPs if you implement buffer > > register/unregister OPs. And there isn't such issue for fused command > > which has same lifetime with the buffer. > > > > > > > > As for keeping files, I predict that it'll be there anyway in some time, > > > some p2pdma experiments, dma preregistration, all required having a file > > > attached to the buffer. > > > > > > > per-file buffer table which isn't supported by io_uring, or it becomes hard to > > > > implement multiple-device io buffer in single context since 'file + buffer key' > > > > has to be used to retrieve this buffer, probably xarray has to be > > > > relied, but > > > > > > I was proposing to give slot selection to the userspace, perhaps with > > > optional auto index allocation as it's done with registered files. > > > > As I mentioned above, it doesn't make sense to export buffer to > > userspace which can't touch any data of the buffer at all. > > replied above. buffer register isn't a must for new io_uring memcpy OP, we can just copy with offset/len & the "buffer". Thanks, Ming
Hello, Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd 64byte SQE(slave) is another normal 64byte OP. For any OP which needs to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1, and its ->issue() can retrieve/import buffer from master request's fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of this idea is from Xiaoguang's ublk ebpf patchset, but this patchset submits slave OP just like normal OP issued from userspace, that said, SQE order is kept, and batching handling is done too. Please see detailed design in commit log of the 2th patch, and one big point is how to handle buffer ownership. With this way, it is easy to support zero copy for ublk/fuse device. Basically userspace can specify any sub-buffer of the ublk block request buffer from the fused command just by setting 'offset/len' in the slave SQE for running slave OP. This way is flexible to implement io mapping: mirror, stripped, ... The 3th & 4th patches enable fused slave support for the following OPs: OP_READ/OP_WRITE OP_SEND/OP_RECV/OP_SEND_ZC The other ublk patches cleans ublk driver and implement fused command for supporting zero copy. Follows userspace code: https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2 All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing: ublk add -t [loop|nbd|qcow2] -z .... Basic fs mount/kernel building and builtin test are done, and also not observe regression on xfstest test over ublk-loop with zero copy. Also add liburing test case for covering fused command based on miniublk of blktest: https://github.com/ming1/liburing/commits/fused_cmd_miniublk Performance improvement is obvious on memory bandwidth related workloads, such as, 1~2X improvement on 64K/512K BS IO test on loop with ramfs backing file. Any comments are welcome! V3: - fix build warning reported by kernel test robot - drop patch for checking fused flags on existed drivers with ->uring_command(), which isn't necessary, since we do not do that when adding new ioctl or uring command - inline io_init_rq() for core code, so just export io_init_slave_req - return result of failed slave request unconditionally since REQ_F_CQE_SKIP will be cleared - pass xfstest over ublk-loop V2: - don't resue io_mapped_ubuf (io_uring) - remove REQ_F_FUSED_MASTER_BIT (io_uring) - fix compile warning (io_uring) - rebase on v6.3-rc1 (io_uring) - grabbing io request reference when handling fused command - simplify ublk_copy_user_pages() by iov iterator - add read()/write() for userspace to read/write ublk io buffer, so that some corner cases(read zero, passthrough request(report zones)) can be handled easily in case of zero copy; this way also helps to switch to zero copy completely - misc cleanup Ming Lei (16): io_uring: increase io_kiocb->flags into 64bit io_uring: add IORING_OP_FUSED_CMD io_uring: support OP_READ/OP_WRITE for fused slave request io_uring: support OP_SEND_ZC/OP_RECV for fused slave request block: ublk_drv: mark device as LIVE before adding disk block: ublk_drv: add common exit handling block: ublk_drv: don't consider flush request in map/unmap io block: ublk_drv: add two helpers to clean up map/unmap request block: ublk_drv: clean up several helpers block: ublk_drv: cleanup 'struct ublk_map_data' block: ublk_drv: cleanup ublk_copy_user_pages block: ublk_drv: grab request reference when the request is handled by userspace block: ublk_drv: support to copy any part of request pages block: ublk_drv: add read()/write() support for ublk char device block: ublk_drv: don't check buffer in case of zero copy block: ublk_drv: apply io_uring FUSED_CMD for supporting zero copy drivers/block/ublk_drv.c | 602 ++++++++++++++++++++++++++------- include/linux/io_uring.h | 49 ++- include/linux/io_uring_types.h | 80 +++-- include/uapi/linux/io_uring.h | 1 + include/uapi/linux/ublk_cmd.h | 37 +- io_uring/Makefile | 2 +- io_uring/fused_cmd.c | 245 ++++++++++++++ io_uring/fused_cmd.h | 11 + io_uring/io_uring.c | 28 +- io_uring/io_uring.h | 3 + io_uring/net.c | 30 +- io_uring/opdef.c | 17 + io_uring/opdef.h | 2 + io_uring/rw.c | 20 ++ 14 files changed, 967 insertions(+), 160 deletions(-) create mode 100644 io_uring/fused_cmd.c create mode 100644 io_uring/fused_cmd.h