On 1/22/24 18:27, David Laight wrote:
From: Matti Vaittinen
Sent: 22 January 2024 06:51
On 1/19/24 13:56, Subhajit Ghosh wrote:
On 8/1/24 02:52, Jonathan Cameron wrote:
On Thu, 4 Jan 2024 11:34:28 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
The loop based 64bit division may run for a long time when dividend is a
lot bigger than the divider. Replace the division loop by the
div64_u64() which implementation may be significantly faster.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Hmm. Fix or not perf improvement? I'm going to take the middle ground
and leave the fixes tag, but not rush this in.
So applied to the togreg branch of iio.git and for now just pushed out
as testing for 0-day etc to take a look before I rebase that tree after
rc1.
---
I've implemented also a fixup series for supporting rounding of
gains/scales:
https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
ail.com/
That series does also remove the offending loop.
We don't currently have any in-tree users of GTS helpers which would
need the rounding support so pushing the rounding is not urgent (and I
haven't heard of Subjahit whose driver required the rounding). Hence, we
may want to only take this loop fix in for now (?) and reconsider
rounding when someone need that.
Why did I look as this crappy code :-)
Well, we all make mistakes, don't we? ;) Thanks for looking at it though!
I think the change breaks the rounding.
This I do disagree with. AFAICS, the original code does not do any
rounding either. This specific patch was just intended for removing the
loop. It did not add or remove the rounding.
For 'normal' values I think you just want:
return 1 + (max - 1)/scale.
I am sorry but I think I got a bit lost. What we currently have in-tree
is just an integer division. 64bit / 64bit, where no rounding whatsoever
is done. It is implemented with a loop - hence the "funny" if - condition.
What I wanted to do with this patch was to just replace the loop.
I think the "return 1 + (max - 1)/scale" is not equivalent?
The driver submitted by Subjahit was first user which used scale values
which did not match exact integer gains. This is because he used
illuminance channel with the real world Lux values :) So, in order to
avoid manual 'change scale values to be exact multiplies of the gains'
work in driver, I thought of doing the regular rounding in these
helpers. This, however, is not done in this specific patch.
Even if it was, I am not sure "return 1 + (max - 1)/scale" would be
correct? If we have for example max = 7, scale = 6, division
7/6 = 1.166666..., which rounds down to 1.
However, 1 + (7 - 1) / 6
=> 1 + 6 / 6
=> 2.
The 'avoid overflow' test isn't needed if you subtract 1 from max.
Ah, yes. I think our confusion comes from that "overflow"-test. It
indeed is no longer necessary as we no longer need to add anything to
the 'max'. This whole function renders to simple:
if (scale > full || !scale)
return -EINVAL;
return div64_u64(full, scale);
unless we implement the proper rounding. should've noticed this. Thanks
for the heads-up!
(Rather than return (max + scale - 1)/scale; where the add can overflow.
But you do need something to return 1 (or error) if max is zero.
David
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
