Re: [PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

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! ~~