Re: [PATCH] lib/math/rational.c: Fix divide by zero

[Daniel Latypov <dlatypov@google.com>]

On Tue, May 25, 2021 at 2:02 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 24, 2021 at 01:17:48PM -0700, Trent Piepho wrote:
> > On Mon, May 24, 2021 at 3:51 AM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Sat, May 22, 2021 at 05:18:06PM -0700, Trent Piepho wrote:
> > >
> > > This misses the test cases (*). Please, develop them with Daniel.
> > >
> > > *) We usually don't accept changes in the generic libraries without test cases.
> > >
> > > Fixes tag?
> >
> > Is there a bug report on a tracker?  I just got the email from Yigua.
>
> Fixes tag refers to the existing commit that brought the bug.
> Also you may need to add Reported-by tag since Yigua reported it.
>
> ...
>
> > > I think that refactoring may lead us to check first iteration before even going
> > > into the loop. But it's another story and we may do it later (the algo uses
> >
> > I started that, but it had no advantages and some disadvantages.
> >
> > Basically, there are three cases: too large, too small & closest to
> > zero, too small & closest to non-zero.  This code can handle those
> > three cases by adding three branches, if(d1), if(n1), and if(!d1).
> > The truth values we need already exist at this point the algorithm.
> >
> > If it's at the start, then there still needs to be the three branches
> > for each case.  But the values to test must be calculated too.
> >
> > What's more, it's possible that the value is exactly representable in
> > the allowed range.  That's actual appears to be the most common use
> > case, reducing a fraction to lowest terms (*).  By putting the tests
> > in the "terminate because of limits" case, they don't need to happen
> > when "terminate because exact value find" is the result. If the check
> > was first, then it would always happen, even if it wouldn't have been
> > necessary.
> >
> > And the time it took to find this bug shows us that out of bounds
> > inputs are not a common case, so putting that on the hot path by
> > checking it first at the expense of the reducing to lowest terms path
> > doesn't make sense.
>
> Thanks for detailed explanation of your view to the current state of the code.
> As you noticed I am not insisting on refactoring or so, I was rather wondering
> if it can be done in the future. Still we might need some performance tests.
>
> Daniel, does KUnit have a capability to test performance?
> Like running test case 1M times or so and calc average (median?) time of
> execution.

No, it does not.
It also currently lacks an option/flag for running a test multiple times.
So one would have to manually modify the test code itself to handle
that right now.

(One non-option is to call `kunit.py execute` in a loop, which will
avoid build + config overhead, but it still adds more than we'd find
acceptable here).

I don't think this was considered before bc it's unclear what the
performance characteristics of UML would be like compared to a more
"normal" arch. Brendan's current patchset to add QEMU support into
kunit.py makes this a bit better, but still running on a physical
machine is still probably safest.

>
> > (*)  One could write a reduce to lowest terms function with an easier
> > interface.  It could be a trivial one expression wrapper around
> > rational_best_approximation().  It could also be a simpler function,
> > but I think it would still perform the exact same sequence of
> > divisions and moduli, so it wouldn't really make any difference.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>