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! > > I don't see this as fine-tuning, rather simplifying the code. I do agree > that it's not going to make much difference for performance of the loop > itself, as the most common case I think is that we either stay in the > current bio_vec or advance by one. > >> >>> 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. >> > > The WARN_ONCE case does leave it inconsistent, though that's not > supposed to happen, so less of a pitfall there. > But I hope, this WARN_ONCE won't ever happen, but I wouldn't be suprised by code like: last_page = (bv + iter->idx - 1)->page. -- Pavel Begunkov
Attachment:
signature.asc
Description: OpenPGP digital signature
