On Thu, Dec 21, 2017 at 4:21 AM, Andrew Jones <drjones@xxxxxxxxxx> wrote:
> On Wed, Dec 20, 2017 at 04:41:24PM -0500, Shih-Wei Li wrote:
>> On Wed, Dec 20, 2017 at 11:41 AM, Andrew Jones <drjones@xxxxxxxxxx> wrote:
>> > On Tue, Dec 19, 2017 at 08:34:45PM -0500, Shih-Wei Li wrote:
>> >> + while (i < iterations) {
>> >> + sample = test->test_fn();
>> >> + if (sample == 0) {
>> >> + /*
>> >> + * If something went wrong or we had an
>> >> + * overflow, don't count that sample.
>> >> + */
>> >> + printf("cycle count overflow: %lu\n", sample);
>> >> + continue;
>> >
>> > Also mentioned in the other mail, we should change this to something like
>> >
>> > if (sample == 0) {
>> > if (failures++ > MAX_FAILURES) {
>> > printf("%s: Too many cycle count overflows\n", test->name);
>> > return;
>> > }
>> > continue;
>> > }
>> >
>> > Notice the dropping of the sample parameter (we know it's zero) and
>> > the addition of the test name.
>> >
>>
>> I think the chance which the timer counter actually overflows ((c1) >
>> (c2)) should be really really rare (mostly due to that (c1) == (c2)),
>> and we can just ignore the case if it happens. So I wonder if we can
>> leave the COUNT macro as the way it is, and change the print info in
>> the code like the following?
>>
>> if (sample == 0) {
>> if (failures++ > MAX_FAILURES) {
>> printf("%s: Too many overflows, the cost is likely smaller than
>> a cycle count\n", test->name);
>> return;
>> }
>> continue;
>> }
>>
>> Also, what might be a good value for MAX_FAILURES based on your
>> experience? The problem in EOI never happened on the machines I used.
>
> It's always zero on at least one of the platforms I use. So it won't
> matter what MAX_FAILURES is set to, we'll always hit it. I think
> the chance we overflow and get c1 == c2 is almost nil, so we might as
> well report the sample as either zero or one in that case. But whatever
> fixes the time outs is fine by me.
Yes, sounds good to me.
>
>>
>> >> + }
>> >> + cycles += sample;
>> >> + if (min == 0 || min > sample)
>> >> + min = sample;
>> >> + if (max < sample)
>> >> + max = sample;
>> >> + ++i;
>> >> + }
>> >> + } while (cycles < goal);
>> >> + printf("%s:\t avg %lu\t min %llu\t max %llu\n",
>> >> + test->name, cycles / iterations, min, max);
>> >
>> > Please use %<num><type> format parameters in order to put this
>> > output in columns. Also, I think we should convert the cycle
>> > counts to time (us) using the timer frequency.
>> >
>>
>> One thing I noticed here is, since in kvm-unit-tests, printf does not
>> currently support floating points, we may lose precision in the output
>> for tests that have smaller cost. For example, in EOI, I saw the min
>> output on seattle became zero since its cost is smaller than 1us.
>> Outputting in nanoseconds might be an option but this then requires
>> type casting floats to unsigned (the timer frequency is around
>> hundreds of MHz in the hardware I tested) just to print the value.
>
> We shouldn't need floats. Just use nanoseconds for calculations and
> then convert to us.us-fraction manually.
>
> ns = cycles * frq;
> us = ns / 1000;
> us_frac = (ns % 1000) / 100; // one decimal place, use 10 for two
> printf("%d.d\n", us, us_frac);
>
Just to make sure, the frq here is 1 / (get_cntfrq() / 10^9), right?
On my seattle, get_cntfrq() gives me 250000000 (2.5*10^8), which makes
the denominator less than 1, so I guess I'll need float to do the math
first, then cast the result to an unsigned number like the following.
float frq = 1 / (get_cntfrq() / 10^9);
ns = (unsigned) cycles * frq;
....
>>
>> I wonder if it's ok to output the timer frequency/counts as we used to
>> have so the users can do the math themselves?
>
> If you wish. I won't insist on the doing the conversion in the test,
> but it seems easy and helpful enough to do.
>
>> >> +# Exit tests
>> >> +[micro-test]
>> >> +file = micro-test.flat
>> >> +smp = 2
>> >> +groups = micro-test
>> >> +accel = kvm
>> >
>> > Unless we come up with some expected values and thresholds for
>> > these execution times in order to turn the results into PASS/FAIL
>> > tests, then I'm still thinking we should probably add it to the
>> > nodefault group in order to save time running the regression tests.
>>
>> I'm ok with making the micro test as a nodefault if we cannot find a
>> generic timeout value that works across the hardware.
>>
>
> OK, let's do that for now.
Yes. Should I just put a line like "groups = nodefault,micro-test" in
unittests.cfg?
Thanks,
Shih-Wei
>
> Thanks,
> drew
