[Qemu-devel] [PATCH 0/2] ide-test: tidy up after pio race fix

[Qemu-devel] [PATCH 0/2] ide-test: tidy up after pio race fix

[John Snow]

Two things:

(1) Fix the timeouts to be more deterministic, and
(2) Finish tidying up the PIO loop.

John Snow (2):
  ide-test: fix timeouts
  ide-test: cdrom_pio_impl fixup

 tests/ide-test.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

-- 
2.4.3

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

[John Snow]

Use explicit timeouts instead of trying to fudge
it by counting nsleep calls.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 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

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

[Kevin Wolf]

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?

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

>  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
> 

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

[John Snow]

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
>>
> 

[Qemu-devel] [PATCH 2/2] ide-test: cdrom_pio_impl fixup

[John Snow]

Final tidying: move the interrupt wait into the loop,
document that the status read clears the IRQ, and move
the final interrupt check outside of the loop.

This should be functionally equivalent to how it works
currently, but a little less ambiguous and slightly more
explicit about the state transitions.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index d37dc5e..f94c859 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -719,9 +719,6 @@ static void cdrom_pio_impl(int nblocks)
     /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
     send_scsi_cdb_read10(0, nblocks);
 
-    /* HP3: INTRQ_Wait */
-    ide_wait_intr(IDE_PRIMARY_IRQ);
-
     /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
      * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
      * We allow an odd limit only when the remaining transfer size is
@@ -733,16 +730,25 @@ static void cdrom_pio_impl(int nblocks)
     for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
         size_t offset = i * (limit / 2);
         size_t rem = (rxsize / 2) - offset;
-        /* HP2: Check_Status_B */
+
+        /* HP3: INTRQ_Wait */
+        ide_wait_intr(IDE_PRIMARY_IRQ);
+
+        /* HP2: Check_Status_B (and clear IRQ) */
         data = ide_wait_clear(BSY);
         assert_bit_set(data, DRQ | DRDY);
         assert_bit_clear(data, ERR | DF | BSY);
+
         /* HP4: Transfer_Data */
         for (j = 0; j < MIN((limit / 2), rem); j++) {
             rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
         }
-        ide_wait_intr(IDE_PRIMARY_IRQ);
     }
+
+    /* Check for final completion IRQ */
+    ide_wait_intr(IDE_PRIMARY_IRQ);
+
+    /* Sanity check final state */
     data = ide_wait_clear(DRQ);
     assert_bit_set(data, DRDY);
     assert_bit_clear(data, DRQ | ERR | DF | BSY);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/2] ide-test: cdrom_pio_impl fixup

[Kevin Wolf]

Am 20.11.2015 um 23:53 hat John Snow geschrieben:
> Final tidying: move the interrupt wait into the loop,
> document that the status read clears the IRQ, and move
> the final interrupt check outside of the loop.
> 
> This should be functionally equivalent to how it works
> currently, but a little less ambiguous and slightly more
> explicit about the state transitions.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>