On Thu, Dec 21, 2017 at 6:28 AM, Christoffer Dall
<christoffer.dall@xxxxxxxxxx> wrote:
> On Thu, Dec 21, 2017 at 10:21:29AM +0100, Andrew Jones 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.
>>
>
> I think the overflow thing should be kept as a sanity check; if we don't
> see a reasonable cycle count (either overflow or c1 == c2), just cancel
> the test and report to the user that this thing is unreliable and they
> should fix their hypervisor. Thoughts?
I guess you meant that we should simply cancel the test whenever we
see an unreasonable cycle count, instead of canceling it until this
happens for MAX_FAILURES of times, yes?
>
>> >
>> > >> + }
>> > >> + 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);
>>
>> >
>> > 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.
>>
>
> I think we're asking for trouble if we output the raw timer counts as
> I'm not sure people are aware this can vary across platforms.
>
> What is the argument against scaling the count to a reasonable measure
> of time?
Because we cannot output float from printf directly in kvm-unit-tests,
so I was just wondering whether we should do the extra conversions to
output the measure of time, or should we leave it for the user to
convert the results.
Thanks,
Shih-Wei
>
> Thanks,
> -Christoffer
