Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] block: optimise bvec_iter_advance()
To:     Arvind Sankar <nivedita@alum.mit.edu>,
        Pavel Begunkov <asml.silence@gmail.com>
Cc:     Ming Lei <ming.lei@redhat.com>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <cover.1574974574.git.asml.silence@gmail.com>
 <06b1b796b8d9bcaa6d5b325668525b7a5663035b.1574974574.git.asml.silence@gmail.com>
 <20191129221709.GA1164864@rani.riverdale.lan>
 <71864178-27d6-c6fb-a66b-395dc46041ac@gmail.com>
 <20191129232445.GA1331087@rani.riverdale.lan>
 <7be4b7fb-5c14-3c3a-e7f1-c5cc6c047f60@gmail.com>
 <20191130185634.GA1848835@rani.riverdale.lan>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <42c183af-2fcf-7d72-70e1-a7a31ab541e5@kernel.dk>
Date:   Sat, 30 Nov 2019 10:57:56 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.1
MIME-Version: 1.0
In-Reply-To: <20191130185634.GA1848835@rani.riverdale.lan>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/30/19 10:56 AM, Arvind Sankar wrote:
> On Sat, Nov 30, 2019 at 12:22:27PM +0300, Pavel Begunkov wrote:
>> On 30/11/2019 02:24, Arvind Sankar wrote:
>>> On Sat, Nov 30, 2019 at 01:47:16AM +0300, Pavel Begunkov wrote:
>>>> 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.
>>>>
>>>
>>> It should be more register-friendly, as it uses fewer variables, and I
>>> think it's easier to see what the loop is doing, i.e. that we advance
>>> one bio_vec per iteration: in the existing code, it takes a bit of
>>> thinking to see that we won't spend more than one iteration within the
>>> same bio_vec.
>>
>> Yeah, may be. It's more the matter of preference then. I don't think
>> it's simpler, and performance is entirely depends on a compiler and
>> input. But, that's rather subjective and IMHO not worth of time.
>>
>> Anyway, thanks for thinking this through!
>>
> 
> You don't find listing 1 simpler than listing 2? It does save one
> register, as it doesn't have to keep track of done independently from
> bytes. This is always going to be the case unless the compiler can
> eliminate done by transforming Listing 2 into Listing 1. Unfortunately,
> even if it gets much smarter, it's unlikely to be able to do that,
> because they're equivalent only if there is no overflow, so it would
> need to know that bytes + iter->bi_bvec_done cannot overflow, and that
> iter->bi_bvec_done must be smaller than cur->bv_len initially.
> 
> Listing 1:
> 
> 	bytes += iter->bi_bvec_done;
> 	while (bytes) {
> 		const struct bio_vec *cur = bv + idx;
> 
> 		if (bytes < cur->bv_len)
> 			break;
> 		bytes -= cur->bv_len;
> 		idx++;
> 	}
> 
> 	iter->bi_idx = idx;
> 	iter->bi_bvec_done = bytes;
> 
> Listing 2:
> 
> 	while (bytes) {
> 		const struct bio_vec *cur = bv + idx;
> 		unsigned int len = min(bytes, cur->bv_len - done);
> 
> 		bytes -= len;
> 		done += len;
> 		if (done == cur->bv_len) {
> 			idx++;
> 			done = 0;
> 		}
> 	}
> 
> 	iter->bi_idx = idx;
> 	iter->bi_bvec_done = done;

Have yet to take a closer look (and benchmark) and the patches and
the generated code, but fwiw I do agree that case #1 is easier to
read.

-- 
Jens Axboe