Re: [PATCH] block: optimise bvec_iter_advance()

[Pavel Begunkov]

On 30/11/2019 01:17, Arvind Sankar wrote:
> 
> The loop can be simplified a bit further, as done has to be 0 once we go
> beyond the current bio_vec. See below for the simplified version.
> 

Thanks for the suggestion! I thought about it, and decided to not
for several reasons. I prefer to not fine-tune and give compilers
more opportunity to do their job. And it's already fast enough with
modern architectures (MOVcc, complex addressing, etc).

Also need to consider code clarity and the fact, that this is inline,
so should be brief and register-friendly.


> I also check if bi_size became zero so we can skip the rest of the
> calculations in that case. If we want to preserve the current behavior of
> updating iter->bi_idx and iter->bi_bvec_done even if bi_size is going to
> become zero, the loop condition should change to
> 
> 		while (bytes && bytes >= cur->bv_len)

Probably, it's better to leave it in a consistent state. Shouldn't be
a problem, but never know when and who will screw it up. 

> 
> to ensure that we don't try to access past the end of the bio_vec array.
> 
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index a032f01e928c..380d188dfbd2 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -87,25 +87,26 @@ struct bvec_iter_all {
>  static inline bool bvec_iter_advance(const struct bio_vec *bv,
>  		struct bvec_iter *iter, unsigned bytes)
>  {
> +	unsigned int idx = iter->bi_idx;
> +	const struct bio_vec *cur = bv + idx;
> +
>  	if (WARN_ONCE(bytes > iter->bi_size,
>  		     "Attempted to advance past end of bvec iter\n")) {
>  		iter->bi_size = 0;
>  		return false;
>  	}
>  
> -	while (bytes) {
> -		const struct bio_vec *cur = bv + iter->bi_idx;
> -		unsigned len = min3(bytes, iter->bi_size,
> -				    cur->bv_len - iter->bi_bvec_done);
> -
> -		bytes -= len;
> -		iter->bi_size -= len;
> -		iter->bi_bvec_done += len;
> -
> -		if (iter->bi_bvec_done == cur->bv_len) {
> -			iter->bi_bvec_done = 0;
> -			iter->bi_idx++;
> +	iter->bi_size -= bytes;
> +	if (iter->bi_size != 0) {
> +		bytes += iter->bi_bvec_done;
> +		while (bytes >= cur->bv_len) {
> +			bytes -= cur->bv_len;
> +			idx++;
> +			cur++;
>  		}
> +
> +		iter->bi_idx = idx;
> +		iter->bi_bvec_done = bytes;
>  	}
>  	return true;
>  }
> 

-- 
Pavel Begunkov

Attachment: signature.asc
Description: OpenPGP digital signature