| Message ID | Zcn-6e-0-nh2WcfU@drtxq0yyyyyyyyyyyyyby-3.rev.dnainternet.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RESEND,v2] iio: gts-helper: Fix division loop | expand |
On Mon, 12 Feb 2024 13:20:09 +0200 Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") > > --- > This is a resend. Only change is the base which is now the v6.8-rc4 and > not the v6.8-rc1 Given I'm not rushing this in, it is going via my togreg tree, so the rebase wasn't really helpful (thankfully didn't stop it applying). Would have been fine to send a ping response to the first posting of it. I was leaving some time for David or Subhajit to have time to take another look, but guess they are either happy with this or busy. Applied to the togreg branch of iio.git and pushed out as testing for all the normal reasons. Jonathan > > This change was earlier applied and reverted as it confusingly lacked of > the removal of the overflow check (which is only needed when we do > looping "while (full > scale * (u64)tmp)". As this loop got removed, the > check got also obsolete and leaving it to the code caused some > confusion. > > So, I marked this as a v2, where v1 is the reverted change discussed > here: > https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/ > > Revision history: > v1 => v2: > - Drop the obsolete overflow check > - Rebased on top of the v6.8-rc4 > > iio: gts: loop fix fix > --- > drivers/iio/industrialio-gts-helper.c | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c > index 7653261d2dc2..b51eb6cb766f 100644 > --- a/drivers/iio/industrialio-gts-helper.c > +++ b/drivers/iio/industrialio-gts-helper.c > @@ -34,24 +34,11 @@ > static int iio_gts_get_gain(const u64 max, const u64 scale) > { > u64 full = max; > - int tmp = 1; > > if (scale > full || !scale) > return -EINVAL; > > - if (U64_MAX - full < scale) { > - /* Risk of overflow */ > - if (full - scale < scale) > - return 1; > - > - full -= scale; > - tmp++; > - } > - > - while (full > scale * (u64)tmp) > - tmp++; > - > - return tmp; > + return div64_u64(full, scale); > } > > /** > > base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
On 17/2/24 00:28, Jonathan Cameron wrote: > On Mon, 12 Feb 2024 13:20:09 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >> >> --- >> This is a resend. Only change is the base which is now the v6.8-rc4 and >> not the v6.8-rc1 > Given I'm not rushing this in, it is going via my togreg tree, so the > rebase wasn't really helpful (thankfully didn't stop it applying). > Would have been fine to send a ping response to the first posting of it. > > I was leaving some time for David or Subhajit to have time to take > another look, but guess they are either happy with this or busy. > > Applied to the togreg branch of iio.git and pushed out as testing for > all the normal reasons. > > Jonathan > >> >> This change was earlier applied and reverted as it confusingly lacked of >> the removal of the overflow check (which is only needed when we do >> looping "while (full > scale * (u64)tmp)". As this loop got removed, the >> check got also obsolete and leaving it to the code caused some >> confusion. >> >> So, I marked this as a v2, where v1 is the reverted change discussed >> here: >> https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/ >> >> Revision history: >> v1 => v2: >> - Drop the obsolete overflow check >> - Rebased on top of the v6.8-rc4 >> >> iio: gts: loop fix fix >> --- >> drivers/iio/industrialio-gts-helper.c | 15 +-------------- >> 1 file changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c >> index 7653261d2dc2..b51eb6cb766f 100644 >> --- a/drivers/iio/industrialio-gts-helper.c >> +++ b/drivers/iio/industrialio-gts-helper.c >> @@ -34,24 +34,11 @@ >> static int iio_gts_get_gain(const u64 max, const u64 scale) >> { >> u64 full = max; >> - int tmp = 1; >> >> if (scale > full || !scale) >> return -EINVAL; >> >> - if (U64_MAX - full < scale) { >> - /* Risk of overflow */ >> - if (full - scale < scale) >> - return 1; >> - >> - full -= scale; >> - tmp++; >> - } >> - >> - while (full > scale * (u64)tmp) >> - tmp++; >> - >> - return tmp; >> + return div64_u64(full, scale); >> } >> >> /** Hi Matti and Jonathan, I somehow missed testing this patch earlier. The above patch works fine with apds9306 v7 driver(which work in progress!). There are no errors. My test script is simple: #!/bin/bash D=0 S=`cat /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale_available` for s in $S; do echo $s echo $s > /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale sleep 5 done One question - if I test a patch like this, do I put a "Tested-by" tag or just mention that I have tested it? Regards, Subhajit Ghosh >> >> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de >
On Sun, 18 Feb 2024 01:09:33 +1030 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > On 17/2/24 00:28, Jonathan Cameron wrote: > > On Mon, 12 Feb 2024 13:20:09 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > >> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") > >> > >> --- > >> This is a resend. Only change is the base which is now the v6.8-rc4 and > >> not the v6.8-rc1 > > Given I'm not rushing this in, it is going via my togreg tree, so the > > rebase wasn't really helpful (thankfully didn't stop it applying). > > Would have been fine to send a ping response to the first posting of it. > > > > I was leaving some time for David or Subhajit to have time to take > > another look, but guess they are either happy with this or busy. > > > > Applied to the togreg branch of iio.git and pushed out as testing for > > all the normal reasons. > > > > Jonathan > > > >> > >> This change was earlier applied and reverted as it confusingly lacked of > >> the removal of the overflow check (which is only needed when we do > >> looping "while (full > scale * (u64)tmp)". As this loop got removed, the > >> check got also obsolete and leaving it to the code caused some > >> confusion. > >> > >> So, I marked this as a v2, where v1 is the reverted change discussed > >> here: > >> https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/ > >> > >> Revision history: > >> v1 => v2: > >> - Drop the obsolete overflow check > >> - Rebased on top of the v6.8-rc4 > >> > >> iio: gts: loop fix fix > >> --- > >> drivers/iio/industrialio-gts-helper.c | 15 +-------------- > >> 1 file changed, 1 insertion(+), 14 deletions(-) > >> > >> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c > >> index 7653261d2dc2..b51eb6cb766f 100644 > >> --- a/drivers/iio/industrialio-gts-helper.c > >> +++ b/drivers/iio/industrialio-gts-helper.c > >> @@ -34,24 +34,11 @@ > >> static int iio_gts_get_gain(const u64 max, const u64 scale) > >> { > >> u64 full = max; > >> - int tmp = 1; > >> > >> if (scale > full || !scale) > >> return -EINVAL; > >> > >> - if (U64_MAX - full < scale) { > >> - /* Risk of overflow */ > >> - if (full - scale < scale) > >> - return 1; > >> - > >> - full -= scale; > >> - tmp++; > >> - } > >> - > >> - while (full > scale * (u64)tmp) > >> - tmp++; > >> - > >> - return tmp; > >> + return div64_u64(full, scale); > >> } > >> > >> /** > Hi Matti and Jonathan, > > I somehow missed testing this patch earlier. The above patch works fine with apds9306 v7 driver(which work in progress!). > There are no errors. > My test script is simple: > #!/bin/bash > D=0 > S=`cat /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale_available` > > for s in $S; do > echo $s > echo $s > /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale > sleep 5 > done > > One question - if I test a patch like this, do I put a "Tested-by" tag or just mention that I have tested it? Both are useful - so thanks for this email. Preference for a formal tag though as that goes in the git commit and we have a convenient record that both says you tested it + that we should make sure to cc you on related changes as you may well be in a position to test those as well! Thanks, Jonathan > > Regards, > Subhajit Ghosh > > >> > >> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de > > >
On 18/2/24 02:57, Jonathan Cameron wrote: > On Sun, 18 Feb 2024 01:09:33 +1030 > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > >> On 17/2/24 00:28, Jonathan Cameron wrote: >>> On Mon, 12 Feb 2024 13:20:09 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>>> >>>> --- >>>> This is a resend. Only change is the base which is now the v6.8-rc4 and >>>> not the v6.8-rc1 >>> Given I'm not rushing this in, it is going via my togreg tree, so the >>> rebase wasn't really helpful (thankfully didn't stop it applying). >>> Would have been fine to send a ping response to the first posting of it. >>> >>> I was leaving some time for David or Subhajit to have time to take >>> another look, but guess they are either happy with this or busy. >>> >>> Applied to the togreg branch of iio.git and pushed out as testing for >>> all the normal reasons. >>> >>> Jonathan >>> >>>> >>>> This change was earlier applied and reverted as it confusingly lacked of >>>> the removal of the overflow check (which is only needed when we do >>>> looping "while (full > scale * (u64)tmp)". As this loop got removed, the >>>> check got also obsolete and leaving it to the code caused some >>>> confusion. >>>> >>>> So, I marked this as a v2, where v1 is the reverted change discussed >>>> here: >>>> https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/ >>>> >>>> Revision history: >>>> v1 => v2: >>>> - Drop the obsolete overflow check >>>> - Rebased on top of the v6.8-rc4 >>>> >>>> iio: gts: loop fix fix >>>> --- >>>> drivers/iio/industrialio-gts-helper.c | 15 +-------------- >>>> 1 file changed, 1 insertion(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c >>>> index 7653261d2dc2..b51eb6cb766f 100644 >>>> --- a/drivers/iio/industrialio-gts-helper.c >>>> +++ b/drivers/iio/industrialio-gts-helper.c >>>> @@ -34,24 +34,11 @@ >>>> static int iio_gts_get_gain(const u64 max, const u64 scale) >>>> { >>>> u64 full = max; >>>> - int tmp = 1; >>>> >>>> if (scale > full || !scale) >>>> return -EINVAL; >>>> >>>> - if (U64_MAX - full < scale) { >>>> - /* Risk of overflow */ >>>> - if (full - scale < scale) >>>> - return 1; >>>> - >>>> - full -= scale; >>>> - tmp++; >>>> - } >>>> - >>>> - while (full > scale * (u64)tmp) >>>> - tmp++; >>>> - >>>> - return tmp; >>>> + return div64_u64(full, scale); >>>> } >>>> >>>> /** >> Hi Matti and Jonathan, >> >> I somehow missed testing this patch earlier. The above patch works fine with apds9306 v7 driver(which work in progress!). >> There are no errors. >> My test script is simple: >> #!/bin/bash >> D=0 >> S=`cat /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale_available` >> >> for s in $S; do >> echo $s >> echo $s > /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale >> sleep 5 >> done >> >> One question - if I test a patch like this, do I put a "Tested-by" tag or just mention that I have tested it? > Both are useful - so thanks for this email. > > Preference for a formal tag though as that goes in the git commit and we have > a convenient record that both says you tested it + that we should make sure > to cc you on related changes as you may well be in a position to test those > as well! > > Thanks, > > Jonathan > >> >> Regards, >> Subhajit Ghosh >> >>>> >>>> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de >>> >> > Thank you Jonathan for explaining the above. I forgot to mention that the above test is run in parallel with continuous raw reads from another script and event monitoring. As I understand that you have already applied this patch but still, Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> Regards, Subhajit Ghosh
Hi Jonathan, On 2/16/24 15:58, Jonathan Cameron wrote: > On Mon, 12 Feb 2024 13:20:09 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >> >> --- >> This is a resend. Only change is the base which is now the v6.8-rc4 and >> not the v6.8-rc1 > Given I'm not rushing this in, it is going via my togreg tree, so the > rebase wasn't really helpful (thankfully didn't stop it applying). Oh, I didn't think about it. Just thought I'll rebase to the most recent tag. I see the point now that you mentioned it, thanks. > Would have been fine to send a ping response to the first posting of it. Ok. Some maintainers like Mark prefer getting full resend instead of a ping because they don't keep the old messages/patches around. Reacting to ping would require them to go and fetch the patch from lore - while having full resend allows them to apply patch using their normal work-flow. Or, at least I think this is how Mark told me couple of years ago. I must admit that plenty of water has flown through the Oulu-river since that, so maybe this has changed also for them. Anyways, good to know your preference, thanks! > I was leaving some time for David or Subhajit to have time to take > another look, but guess they are either happy with this or busy. Ok. This is perfectly fine. I just thought that maybe the patch fell through the cracks and decided to re-send before I forget ... :) > Applied to the togreg branch of iio.git and pushed out as testing for > all the normal reasons. Thanks! Yours, -- Matti
On 2/18/24 07:26, Subhajit Ghosh wrote: > On 18/2/24 02:57, Jonathan Cameron wrote: >> On Sun, 18 Feb 2024 01:09:33 +1030 >> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: >> >>> On 17/2/24 00:28, Jonathan Cameron wrote: >>>> On Mon, 12 Feb 2024 13:20:09 +0200 >>>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>>>> >>>>> --- > As I understand that you have already applied this patch but still, > > Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> Thank you Subhajit! Your effort is _very much_ appreciated! :) Yours, -- Matti
On Mon, 19 Feb 2024 09:22:24 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 2/18/24 07:26, Subhajit Ghosh wrote: > > On 18/2/24 02:57, Jonathan Cameron wrote: > >> On Sun, 18 Feb 2024 01:09:33 +1030 > >> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > >> > >>> On 17/2/24 00:28, Jonathan Cameron wrote: > >>>> On Mon, 12 Feb 2024 13:20:09 +0200 > >>>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > >>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") > >>>>> > >>>>> --- > > > > As I understand that you have already applied this patch but still, > > > > Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> > > Thank you Subhajit! Your effort is _very much_ appreciated! :) I had to rebase the tree anyway to squash an unrelated issue, so I added the tag whilst doing so. Thanks, Jonathan > > Yours, > -- Matti >
On 20/2/24 06:02, Jonathan Cameron wrote: > On Mon, 19 Feb 2024 09:22:24 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 2/18/24 07:26, Subhajit Ghosh wrote: >>> On 18/2/24 02:57, Jonathan Cameron wrote: >>>> On Sun, 18 Feb 2024 01:09:33 +1030 >>>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: >>>> >>>>> On 17/2/24 00:28, Jonathan Cameron wrote: >>>>>> On Mon, 12 Feb 2024 13:20:09 +0200 >>>>>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >>>>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") >>>>>>> >>>>>>> --- >> >> >>> As I understand that you have already applied this patch but still, >>> >>> Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> >> >> Thank you Subhajit! Your effort is _very much_ appreciated! :) > I had to rebase the tree anyway to squash an unrelated issue, so > I added the tag whilst doing so. > > Thanks, > > Jonathan > >> >> Yours, >> -- Matti >> > You are welcome Matti and thank you Jonathan. Regards, Subhajit Ghosh
diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c index 7653261d2dc2..b51eb6cb766f 100644 --- a/drivers/iio/industrialio-gts-helper.c +++ b/drivers/iio/industrialio-gts-helper.c @@ -34,24 +34,11 @@ static int iio_gts_get_gain(const u64 max, const u64 scale) { u64 full = max; - int tmp = 1; if (scale > full || !scale) return -EINVAL; - if (U64_MAX - full < scale) { - /* Risk of overflow */ - if (full - scale < scale) - return 1; - - full -= scale; - tmp++; - } - - while (full > scale * (u64)tmp) - tmp++; - - return tmp; + return div64_u64(full, scale); } /**
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@gmail.com> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers") --- This is a resend. Only change is the base which is now the v6.8-rc4 and not the v6.8-rc1 This change was earlier applied and reverted as it confusingly lacked of the removal of the overflow check (which is only needed when we do looping "while (full > scale * (u64)tmp)". As this loop got removed, the check got also obsolete and leaving it to the code caused some confusion. So, I marked this as a v2, where v1 is the reverted change discussed here: https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/ Revision history: v1 => v2: - Drop the obsolete overflow check - Rebased on top of the v6.8-rc4 iio: gts: loop fix fix --- drivers/iio/industrialio-gts-helper.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de