From: Pavel Begunkov <asml.silence@gmail.com>
To: Jens Axboe <axboe@kernel.dk>, Arvind Sankar <nivedita@alum.mit.edu>
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] block: optimise bvec_iter_advance()
Date: Sat, 30 Nov 2019 23:11:57 +0300 [thread overview]
Message-ID: <84a065b0-956c-460a-3575-260df7117fb8@gmail.com> (raw)
In-Reply-To: <42c183af-2fcf-7d72-70e1-a7a31ab541e5@kernel.dk>
[-- Attachment #1.1: Type: text/plain, Size: 3226 bytes --]
On 30/11/2019 21:57, Jens Axboe wrote:
> 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.
>
Ok, ok, I'm not keen on bike-shedding. I'll resend a simplified version
--
Pavel Begunkov
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-11-30 20:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1574974574.git.asml.silence@gmail.com>
2019-11-28 21:04 ` [PATCH] block: optimise bvec_iter_advance() Pavel Begunkov
2019-11-29 22:17 ` Arvind Sankar
2019-11-29 22:47 ` Pavel Begunkov
2019-11-29 23:24 ` Arvind Sankar
2019-11-30 9:22 ` Pavel Begunkov
2019-11-30 18:56 ` Arvind Sankar
2019-11-30 18:57 ` Jens Axboe
2019-11-30 20:11 ` Pavel Begunkov [this message]
2019-11-30 20:56 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84a065b0-956c-460a-3575-260df7117fb8@gmail.com \
--to=asml.silence@gmail.com \
--cc=axboe@kernel.dk \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nivedita@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).