From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpGfK-0004Te-54 for qemu-devel@nongnu.org; Thu, 12 Jul 2012 06:30:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SpGfC-0004Wr-A8 for qemu-devel@nongnu.org; Thu, 12 Jul 2012 06:30:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30263) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SpGfC-0004WX-2G for qemu-devel@nongnu.org; Thu, 12 Jul 2012 06:30:26 -0400 Message-ID: <4FFEA738.6040807@redhat.com> Date: Thu, 12 Jul 2012 12:30:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1341501390-797-1-git-send-email-pbonzini@redhat.com> <1341501390-797-3-git-send-email-pbonzini@redhat.com> <4FFE9471.1060305@redhat.com> In-Reply-To: <4FFE9471.1060305@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH uq/master 2/9] event_notifier: remove event_notifier_test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: kvm@vger.kernel.org, mst@redhat.com, jan.kiszka@siemens.com, mtosatti@redhat.com, qemu-devel@nongnu.org, anthony.perard@citrix.com, stefano.stabellini@eu.citrix.com Il 12/07/2012 11:10, Avi Kivity ha scritto: > On 07/05/2012 06:16 PM, Paolo Bonzini wrote: >> This is broken; since the eventfd is used in nonblocking mode there >> is a race between reading and writing. >> > >> diff --git a/event_notifier.c b/event_notifier.c >> index 2b210f4..c339bfe 100644 >> --- a/event_notifier.c >> +++ b/event_notifier.c >> @@ -51,18 +51,3 @@ int event_notifier_test_and_clear(EventNotifier *e) >> int r = read(e->fd, &value, sizeof(value)); >> return r == sizeof(value); >> } >> - >> -int event_notifier_test(EventNotifier *e) >> -{ >> - uint64_t value; >> - int r = read(e->fd, &value, sizeof(value)); >> - if (r == sizeof(value)) { >> - /* restore previous value. */ >> - int s = write(e->fd, &value, sizeof(value)); >> - /* never blocks because we use EFD_SEMAPHORE. >> - * If we didn't we'd get EAGAIN on overflow >> - * and we'd have to write code to ignore it. */ >> - assert(s == sizeof(value)); >> - } >> - return r == sizeof(value); >> -} > > I don't see the race. Mind explaining? The assertion can actually fire, there's nothing that prevents this from happening: event_notifier_test() read(fd, &value, 8) write(fd, , 8) write(fd, &value, 8) event_notifier_set will always write a 1 and it will take a large amount of writes to reach overflow :) but that may not be true of other writers using the same file descriptor. Then, the comment is wrong in two ways. First, we do not use EFD_SEMAPHORE (though even if we did the only difference is that value will be always one). Second, we cannot write code to ignore EAGAIN, because then we've lost the value. With blocking I/O things would not be much better, because then event_notifier_test() might block on the write. That would be quite surprising. If we cared, we could implement the function more easily and corectly with poll(), checking for POLLIN in the revents. But I don't see a sensible use case for it anyway. Paolo