From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 0/2] eventfd: new EFD_STATE flag Date: Sun, 10 Jan 2010 19:35:57 +0200 Message-ID: <20100110173557.GA29350@redhat.com> References: <20100106234521.GA30515@redhat.com> <20100107064503.GA10379@redhat.com> <20100107103600.GD599@redhat.com> <20100110103007.GA20538@redhat.com> <20100110162202.GA29263@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm@vger.kernel.org To: Davide Libenzi Return-path: Received: from mx1.redhat.com ([209.132.183.28]:29371 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751350Ab0AJRix (ORCPT ); Sun, 10 Jan 2010 12:38:53 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Sun, Jan 10, 2010 at 09:27:34AM -0800, Davide Libenzi wrote: > On Sun, 10 Jan 2010, Michael S. Tsirkin wrote: > > > Not sure what you mean by irqfd locking. IMO we must take waitqueue > > lock, otherwise we can not prevent wait queue callback from being > > invoked, right? Note that if we take our own lock under wait queue > > callback, we won't be able to use this lock around remove_wait_queue. > > Also note how fs/evetfd.c uses __remove_wait_queue after taking wait > > queue lock - we'd like to do the same with our wait queue. > > > > Could you clarify pls? > > Wait a second. Looking at the current code in Linus tree, when you > deassign an irqfd, you are actually destroying it, so the idea of saving > the counter on deassign is not going to work. > > > > - Davide Yes, the only context passed between deassign and assign is the eventfd itself. So I think the following code for deassign would work provided eventfd_ctx_read_locked works with wait queue lock taken: spin_lock_irqsave(&irqfd->wqh.lock, flags); eventfd_ctx_read_locked(ctx->eventfd, 1, &ucnt); __remove_wait_queue(irqfd->wqh, &irqfd->wait); spin_lock_irqrestore(&irqfd->wqh.lock, flags); And we discard ucnt here. This works because on on assign we do: events = file->f_op->poll(file, &irqfd->pt); if (events & POLLIN) schedule_work(&irqfd->inject); -- MST