From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40557) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1HIE-0002RC-Eo for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:22:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1HI9-0002ct-DS for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:22:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44539) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1HI9-0002cp-7p for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:22:09 -0500 References: <1448060035-31973-1-git-send-email-jsnow@redhat.com> <1448060035-31973-2-git-send-email-jsnow@redhat.com> <20151124154018.GH4296@noname.str.redhat.com> From: John Snow Message-ID: <56549CC0.7050000@redhat.com> Date: Tue, 24 Nov 2015 12:22:08 -0500 MIME-Version: 1.0 In-Reply-To: <20151124154018.GH4296@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] ide-test: fix timeouts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pl@kamp.de, qemu-devel@nongnu.org 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 > > 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 >> >