On 3/27/23 12:42?PM, Al Viro wrote: > On Mon, Mar 27, 2023 at 12:01:08PM -0600, Jens Axboe wrote: >> On 3/24/23 10:46?PM, Al Viro wrote: >>> On Fri, Mar 24, 2023 at 02:44:41PM -0600, Jens Axboe wrote: >>>> Hi, >>>> >>>> We've been doing a few conversions of ITER_IOVEC to ITER_UBUF in select >>>> spots, as the latter is cheaper to iterate and hence saves some cycles. >>>> I recently experimented [1] with io_uring converting single segment READV >>>> and WRITEV into non-vectored variants, as we can save some cycles through >>>> that as well. >>>> >>>> But there's really no reason why we can't just do this further down, >>>> enabling it for everyone. It's quite common to use vectored reads or >>>> writes even with a single segment, unfortunately, even for cases where >>>> there's no specific reason to do so. From a bit of non-scientific >>>> testing on a vm on my laptop, I see about 60% of the import_iovec() >>>> calls being for a single segment. >>>> >>>> I initially was worried that we'd have callers assuming an ITER_IOVEC >>>> iter after a call import_iovec() or import_single_range(), but an audit >>>> of the kernel code actually looks sane in that regard. Of the ones that >>>> do call it, I ran the ltp test cases and they all still pass. >>> >>> Which tree was that audit on? Mainline? Some branch in block.git? >> >> It was just master in -git. But looks like I did miss two spots, I've >> updated the series here and will send out a v2: >> >> https://git.kernel.dk/cgit/linux-block/log/?h=iter-ubuf > > Just to make sure - head's at 4d0ba2f0250d? Correct! > One obvious comment (just about the problems you've dealt with in that > branch; I'll go over that tree and look for other sources of trouble, > will post tonight): > > all 3 callers of iov_iter_iovec() in there are accompanied by the identical > chunks that deal with ITER_UBUF case; it would make more sense to teach > iov_iter_iovec() to handle that. loop_rw_iter() would turn into > if (!iov_iter_is_bvec(iter)) { > iovec = iov_iter_iovec(iter); > } else { > iovec.iov_base = u64_to_user_ptr(rw->addr); > iovec.iov_len = rw->len; > } > and process_madvise() and do_loop_readv_writev() patches simply go away. > > Again, I'm _not_ saying there's no other problems left, just that these are > better dealt with that way. > > Something like > > static inline struct iovec iov_iter_iovec(const struct iov_iter *iter) > { > if (WARN_ON(!iter->user_backed)) > return (struct iovec) { > .iov_base = NULL, > .iov_len = 0 > }; > else if (iov_iter_is_ubuf(iter)) > return (struct iovec) { > .iov_base = iter->ubuf + iter->iov_offset, > .iov_len = iter->count > }; > else > return (struct iovec) { > .iov_base = iter->iov->iov_base + iter->iov_offset, > .iov_len = min(iter->count, > iter->iov->iov_len - iter->iov_offset), > }; > } > > and no need to duplicate that logics in all callers. Or get rid of > those elses, seeing that each alternative is a plain return - matter > of taste... That's a great idea. Two questions - do we want to make that WARN_ON_ONCE()? And then do we want to include a WARN_ON_ONCE for a non-supported type? Doesn't seem like high risk as they've all been used with ITER_IOVEC until now, though. -- Jens Axboe
