Re: [Qemu-devel] [PATCH 1/2] ide-test: fix timeouts

[John Snow <jsnow@redhat.com>]

On 11/24/2015 10:40 AM, Kevin Wolf wrote:
> Am 20.11.2015 um 23:53 hat John Snow geschrieben:
>> Use explicit timeouts instead of trying to fudge
>> it by counting nsleep calls.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Isn't wrapping lines at 50 characters a bit extreme?
> 

Yes. Sometimes, I guess conservatively...

> I'm wondering why this is a fix. If anything, I would expect the new
> version to be less precise because its resolution is much coarser
> (1 s vs. 400 ns). Should still be good enough, so both the new and the
> old code look okay to me, but I'd appreciate a commit message that tells
> the motivation for this change.
> 
> Kevin
> 

The old code, in practice, doesn't actually time out in even under a
minute. If this is anywhere from (4-6) seconds, it's an improvement. I
could set the timer to something like 6 seconds to make sure we're
always giving it a full five.

I was counting the nsleep calls as the total timeout, but it ignores all
of the time it spends in the inb() call, which in practice dwarfs the
nsleep timer. So instead of 5 seconds we got something much, much larger.

The new code will fail the test far, far sooner.

I can respin with this explanation.

--js

>>  tests/ide-test.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>> index fc1ce52..d37dc5e 100644
>> --- a/tests/ide-test.c
>> +++ b/tests/ide-test.c
>> @@ -642,15 +642,20 @@ static void nsleep(int64_t nsecs)
>>  
>>  static uint8_t ide_wait_clear(uint8_t flag)
>>  {
>> -    int i;
>>      uint8_t data;
>> +    time_t st, now;
>>  
>>      /* Wait with a 5 second timeout */
>> -    for (i = 0; i <= 12500000; i++) {
>> +    time(&st);
>> +    while (true) {
>>          data = inb(IDE_BASE + reg_status);
>>          if (!(data & flag)) {
>>              return data;
>>          }
>> +        time(&now);
>> +        if (difftime(now, st) > 5.0) {
>> +            break;
>> +        }
>>          nsleep(400);
>>      }
>>      g_assert_not_reached();
>> @@ -658,14 +663,19 @@ static uint8_t ide_wait_clear(uint8_t flag)
>>  
>>  static void ide_wait_intr(int irq)
>>  {
>> -    int i;
>> +    time_t st, now;
>>      bool intr;
>>  
>> -    for (i = 0; i <= 12500000; i++) {
>> +    time(&st);
>> +    while (true) {
>>          intr = get_irq(irq);
>>          if (intr) {
>>              return;
>>          }
>> +        time(&now);
>> +        if (difftime(now, st) > 5.0) {
>> +            break;
>> +        }
>>          nsleep(400);
>>      }
>>  
>> -- 
>> 2.4.3
>>
>