Re: [PATCH] iov_iter: optimise iter type checking

[Pavel Begunkov <asml.silence@gmail.com>]

On 16/01/2021 05:18, Al Viro wrote:
> On Sat, Jan 09, 2021 at 10:11:09PM +0000, Pavel Begunkov wrote:
> 
>>> Does any code actually look at the fields as a pair?
>>> Would it even be better to use separate bytes?
>>> Even growing the on-stack structure by a word won't really matter.
>>
>> u8 type, rw;
>>
>> That won't bloat the struct. I like the idea. If used together compilers
>> can treat it as u16.
> 
> Reasonable, and from what I remember from looking through the users,
> no readers will bother with looking at both at the same time.

Al, are you going turn it into a patch, or prefer me to take over?


> On the write side... it's only set in iov_iter_{kvec,bvec,pipe,discard,init}.
> I sincerely doubt anyone would give a fuck, not to mention that something
> like
> void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
>                         struct pipe_inode_info *pipe,
>                         size_t count)
> {
>         BUG_ON(direction != READ);
>         WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
> 	*i = (struct iov_iter) {
> 		.iter_type = ITER_PIPE,
> 		.data_source = false,
> 		.pipe = pipe,
> 		.head = pipe->head,
> 		.start_head = pipe->head,
> 		.count = count,
> 		.iov_offset = 0
> 	};
> }
> 
> would make more sense anyway - we do want to overwrite everything in the
> object, and let the compiler do whatever it likes to do.
> 
> So... something like (completely untested) variant below, perhaps?
> 
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..ed8ad2c5d384 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -19,20 +19,16 @@ struct kvec {
>  
>  enum iter_type {
>  	/* iter types */
> -	ITER_IOVEC = 4,
> -	ITER_KVEC = 8,
> -	ITER_BVEC = 16,
> -	ITER_PIPE = 32,
> -	ITER_DISCARD = 64,
> +	ITER_IOVEC,
> +	ITER_KVEC,
> +	ITER_BVEC,
> +	ITER_PIPE,
> +	ITER_DISCARD
>  };
>  
>  struct iov_iter {
> -	/*
> -	 * Bit 0 is the read/write bit, set if we're writing.
> -	 * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
> -	 * the caller isn't expecting to drop a page reference when done.
> -	 */
> -	unsigned int type;
> +	u8 iter_type;
> +	bool data_source;
>  	size_t iov_offset;
>  	size_t count;
>  	union {
> @@ -52,7 +48,7 @@ struct iov_iter {
>  
>  static inline enum iter_type iov_iter_type(const struct iov_iter *i)
>  {
> -	return i->type & ~(READ | WRITE);
> +	return i->iter_type;
>  }
>  
>  static inline bool iter_is_iovec(const struct iov_iter *i)
> @@ -82,7 +78,7 @@ static inline bool iov_iter_is_discard(const struct iov_iter *i)
>  
>  static inline unsigned char iov_iter_rw(const struct iov_iter *i)
>  {
> -	return i->type & (READ | WRITE);
> +	return i->data_source ? WRITE : READ;
>  }
>  
>  /*
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 1635111c5bd2..133c03b2dcae 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -81,19 +81,18 @@
>  #define iterate_all_kinds(i, n, v, I, B, K) {			\
>  	if (likely(n)) {					\
>  		size_t skip = i->iov_offset;			\
> -		if (unlikely(i->type & ITER_BVEC)) {		\
> +		if (likely(i->iter_type == ITER_IOVEC)) {	\
> +			const struct iovec *iov;		\
> +			struct iovec v;				\
> +			iterate_iovec(i, n, v, iov, skip, (I))	\
> +		} else if (i->iter_type == ITER_BVEC) {		\
>  			struct bio_vec v;			\
>  			struct bvec_iter __bi;			\
>  			iterate_bvec(i, n, v, __bi, skip, (B))	\
> -		} else if (unlikely(i->type & ITER_KVEC)) {	\
> +		} else if (i->iter_type == ITER_KVEC) {		\
>  			const struct kvec *kvec;		\
>  			struct kvec v;				\
>  			iterate_kvec(i, n, v, kvec, skip, (K))	\
> -		} else if (unlikely(i->type & ITER_DISCARD)) {	\
> -		} else {					\
> -			const struct iovec *iov;		\
> -			struct iovec v;				\
> -			iterate_iovec(i, n, v, iov, skip, (I))	\
>  		}						\
>  	}							\
>  }
> @@ -103,7 +102,17 @@
>  		n = i->count;					\
>  	if (i->count) {						\
>  		size_t skip = i->iov_offset;			\
> -		if (unlikely(i->type & ITER_BVEC)) {		\
> +		if (likely(i->iter_type == ITER_IOVEC)) {	\
> +			const struct iovec *iov;		\
> +			struct iovec v;				\
> +			iterate_iovec(i, n, v, iov, skip, (I))	\
> +			if (skip == iov->iov_len) {		\
> +				iov++;				\
> +				skip = 0;			\
> +			}					\
> +			i->nr_segs -= iov - i->iov;		\
> +			i->iov = iov;				\
> +		} else if (i->iter_type == ITER_BVEC) {		\
>  			const struct bio_vec *bvec = i->bvec;	\
>  			struct bio_vec v;			\
>  			struct bvec_iter __bi;			\
> @@ -111,7 +120,7 @@
>  			i->bvec = __bvec_iter_bvec(i->bvec, __bi);	\
>  			i->nr_segs -= i->bvec - bvec;		\
>  			skip = __bi.bi_bvec_done;		\
> -		} else if (unlikely(i->type & ITER_KVEC)) {	\
> +		} else if (i->iter_type == ITER_KVEC) {		\
>  			const struct kvec *kvec;		\
>  			struct kvec v;				\
>  			iterate_kvec(i, n, v, kvec, skip, (K))	\
> @@ -121,18 +130,8 @@
>  			}					\
>  			i->nr_segs -= kvec - i->kvec;		\
>  			i->kvec = kvec;				\
> -		} else if (unlikely(i->type & ITER_DISCARD)) {	\
> +		} else if (i->iter_type == ITER_DISCARD) {	\
>  			skip += n;				\
> -		} else {					\
> -			const struct iovec *iov;		\
> -			struct iovec v;				\
> -			iterate_iovec(i, n, v, iov, skip, (I))	\
> -			if (skip == iov->iov_len) {		\
> -				iov++;				\
> -				skip = 0;			\
> -			}					\
> -			i->nr_segs -= iov - i->iov;		\
> -			i->iov = iov;				\
>  		}						\
>  		i->count -= n;					\
>  		i->iov_offset = skip;				\
> @@ -434,7 +433,7 @@ int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes)
>  	int err;
>  	struct iovec v;
>  
> -	if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
> +	if (i->iter_type == ITER_IOVEC) {
>  		iterate_iovec(i, bytes, v, iov, skip, ({
>  			err = fault_in_pages_readable(v.iov_base, v.iov_len);
>  			if (unlikely(err))
> @@ -450,19 +449,26 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
>  			size_t count)
>  {
>  	WARN_ON(direction & ~(READ | WRITE));
> -	direction &= READ | WRITE;
>  
>  	/* It will get better.  Eventually... */
> -	if (uaccess_kernel()) {
> -		i->type = ITER_KVEC | direction;
> -		i->kvec = (struct kvec *)iov;
> -	} else {
> -		i->type = ITER_IOVEC | direction;
> -		i->iov = iov;
> -	}
> -	i->nr_segs = nr_segs;
> -	i->iov_offset = 0;
> -	i->count = count;
> +	if (uaccess_kernel())
> +		*i = (struct iov_iter) {
> +			.iter_type = ITER_KVEC,
> +			.data_source = direction,
> +			.kvec = (struct kvec *)iov,
> +			.nr_segs = nr_segs,
> +			.iov_offset = 0,
> +			.count = count
> +		};
> +	else
> +		*i = (struct iov_iter) {
> +			.iter_type = ITER_IOVEC,
> +			.data_source = direction,
> +			.iov = iov,
> +			.nr_segs = nr_segs,
> +			.iov_offset = 0,
> +			.count = count
> +		};
>  }
>  EXPORT_SYMBOL(iov_iter_init);
>  
> @@ -915,17 +921,20 @@ size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
>  {
>  	if (unlikely(!page_copy_sane(page, offset, bytes)))
>  		return 0;
> -	if (i->type & (ITER_BVEC|ITER_KVEC)) {
> +	if (likely(i->iter_type == ITER_IOVEC))
> +		return copy_page_to_iter_iovec(page, offset, bytes, i);
> +	if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
>  		void *kaddr = kmap_atomic(page);
>  		size_t wanted = copy_to_iter(kaddr + offset, bytes, i);
>  		kunmap_atomic(kaddr);
>  		return wanted;
> -	} else if (unlikely(iov_iter_is_discard(i)))
> -		return bytes;
> -	else if (likely(!iov_iter_is_pipe(i)))
> -		return copy_page_to_iter_iovec(page, offset, bytes, i);
> -	else
> +	}
> +	if (i->iter_type == ITER_PIPE)
>  		return copy_page_to_iter_pipe(page, offset, bytes, i);
> +	if (i->iter_type == ITER_DISCARD)
> +		return bytes;
> +	WARN_ON(1);
> +	return 0;
>  }
>  EXPORT_SYMBOL(copy_page_to_iter);
>  
> @@ -934,17 +943,16 @@ size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
>  {
>  	if (unlikely(!page_copy_sane(page, offset, bytes)))
>  		return 0;
> -	if (unlikely(iov_iter_is_pipe(i) || iov_iter_is_discard(i))) {
> -		WARN_ON(1);
> -		return 0;
> -	}
> -	if (i->type & (ITER_BVEC|ITER_KVEC)) {
> +	if (likely(i->iter_type == ITER_IOVEC))
> +		return copy_page_from_iter_iovec(page, offset, bytes, i);
> +	if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
>  		void *kaddr = kmap_atomic(page);
>  		size_t wanted = _copy_from_iter(kaddr + offset, bytes, i);
>  		kunmap_atomic(kaddr);
>  		return wanted;
> -	} else
> -		return copy_page_from_iter_iovec(page, offset, bytes, i);
> +	}
> +	WARN_ON(1);
> +	return 0;
>  }
>  EXPORT_SYMBOL(copy_page_from_iter);
>  
> @@ -1172,11 +1180,14 @@ void iov_iter_kvec(struct iov_iter *i, unsigned int direction,
>  			size_t count)
>  {
>  	WARN_ON(direction & ~(READ | WRITE));
> -	i->type = ITER_KVEC | (direction & (READ | WRITE));
> -	i->kvec = kvec;
> -	i->nr_segs = nr_segs;
> -	i->iov_offset = 0;
> -	i->count = count;
> +	*i = (struct iov_iter) {
> +		.iter_type = ITER_KVEC,
> +		.data_source = direction,
> +		.kvec = kvec,
> +		.nr_segs = nr_segs,
> +		.iov_offset = 0,
> +		.count = count
> +	};
>  }
>  EXPORT_SYMBOL(iov_iter_kvec);
>  
> @@ -1185,11 +1196,14 @@ void iov_iter_bvec(struct iov_iter *i, unsigned int direction,
>  			size_t count)
>  {
>  	WARN_ON(direction & ~(READ | WRITE));
> -	i->type = ITER_BVEC | (direction & (READ | WRITE));
> -	i->bvec = bvec;
> -	i->nr_segs = nr_segs;
> -	i->iov_offset = 0;
> -	i->count = count;
> +	*i = (struct iov_iter) {
> +		.iter_type = ITER_BVEC,
> +		.data_source = direction,
> +		.bvec = bvec,
> +		.nr_segs = nr_segs,
> +		.iov_offset = 0,
> +		.count = count
> +	};
>  }
>  EXPORT_SYMBOL(iov_iter_bvec);
>  
> @@ -1199,12 +1213,15 @@ void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
>  {
>  	BUG_ON(direction != READ);
>  	WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
> -	i->type = ITER_PIPE | READ;
> -	i->pipe = pipe;
> -	i->head = pipe->head;
> -	i->iov_offset = 0;
> -	i->count = count;
> -	i->start_head = i->head;
> +        *i = (struct iov_iter) {
> +		.iter_type = ITER_PIPE,
> +		.data_source = false,
> +		.pipe = pipe,
> +		.head = pipe->head,
> +		.start_head = pipe->head,
> +		.count = count,
> +		.iov_offset = 0
> +	};
>  }
>  EXPORT_SYMBOL(iov_iter_pipe);
>  
> @@ -1220,9 +1237,11 @@ EXPORT_SYMBOL(iov_iter_pipe);
>  void iov_iter_discard(struct iov_iter *i, unsigned int direction, size_t count)
>  {
>  	BUG_ON(direction != READ);
> -	i->type = ITER_DISCARD | READ;
> -	i->count = count;
> -	i->iov_offset = 0;
> +	*i = (struct iov_iter) {
> +		.iter_type = ITER_DISCARD,
> +		.data_source = false,
> +		.count = count,
> +	};
>  }
>  EXPORT_SYMBOL(iov_iter_discard);
>  
> 

-- 
Pavel Begunkov