Message-ID: <4A37B224.1070804@novell.com>
Date: Tue, 16 Jun 2009 10:54:28 -0400
From: Gregory Haskins <ghaskins@novell.com>
User-Agent: Thunderbird 2.0.0.21 (Macintosh/20090302)
MIME-Version: 1.0
To: "Michael S. Tsirkin" <mst@redhat.com>
CC: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, avi@redhat.com,
       davidel@xmailserver.org, paulmck@linux.vnet.ibm.com, mingo@elte.hu
Subject: Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier
 interface
References: <20090616022041.23890.90120.stgit@dev.haskins.net> <20090616022956.23890.63776.stgit@dev.haskins.net> <20090616140240.GA9401@redhat.com> <4A37A7FC.4090403@novell.com> <20090616143816.GA18196@redhat.com> <4A37B0BB.3020005@novell.com>
In-Reply-To: <4A37B0BB.3020005@novell.com>
Content-Type: multipart/signed; micalg=pgp-sha1;
 protocol="application/pgp-signature";
 boundary="------------enig9D20F261EB862F1551BF1803"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6840
Lines: 200

This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enig9D20F261EB862F1551BF1803
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
>  =20
>> On Tue, Jun 16, 2009 at 10:11:08AM -0400, Gregory Haskins wrote:
>>  =20
>>    =20
>>> Michael S. Tsirkin wrote:
>>>    =20
>>>      =20
>>>> On Mon, Jun 15, 2009 at 10:29:56PM -0400, Gregory Haskins wrote:
>>>>  =20
>>>>      =20
>>>>        =20
>>>>> irqfd and its underlying implementation, eventfd, currently utilize=

>>>>> the embedded wait-queue in eventfd for signal notification.  The ni=
ce thing
>>>>> about this design decision is that it re-uses the existing
>>>>> eventfd/wait-queue code and it generally works well....with several=

>>>>> limitations.
>>>>>
>>>>> One of the limitations is that notification callbacks are always ca=
lled
>>>>> inside a spin_lock_irqsave critical section.  Another limitation is=

>>>>> that it is very difficult to build a system that can recieve releas=
e
>>>>> notification without being racy.
>>>>>
>>>>> Therefore, we introduce a new registration interface that is SRCU b=
ased
>>>>> instead of wait-queue based, and implement the internal wait-queue
>>>>> infrastructure in terms of this new interface.  We then convert irq=
fd
>>>>> to use this new interface instead of the existing wait-queue code.
>>>>>
>>>>> The end result is that we now have the opportunity to run the inter=
rupt
>>>>> injection code serially to the callback (when the signal is raised =
from
>>>>> process-context, at least) instead of always deferring the injectio=
n to a
>>>>> work-queue.
>>>>>
>>>>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>>>>> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>>>>> CC: Davide Libenzi <davidel@xmailserver.org>
>>>>> ---
>>>>>
>>>>>  fs/eventfd.c            |  115 +++++++++++++++++++++++++++++++++++=
++++++++----
>>>>>  include/linux/eventfd.h |   30 ++++++++++++
>>>>>  virt/kvm/eventfd.c      |  114 +++++++++++++++++++++--------------=
------------
>>>>>  3 files changed, 188 insertions(+), 71 deletions(-)
>>>>>
>>>>> diff --git a/fs/eventfd.c b/fs/eventfd.c
>>>>> index 72f5f8d..505d5de 100644
>>>>> --- a/fs/eventfd.c
>>>>> +++ b/fs/eventfd.c
>>>>> @@ -30,8 +30,47 @@ struct eventfd_ctx {
>>>>>  	 */
>>>>>  	__u64 count;
>>>>>  	unsigned int flags;
>>>>> +	struct srcu_struct srcu;
>>>>> +	struct list_head nh;
>>>>> +	struct eventfd_notifier notifier;
>>>>>  };
>>>>> =20
>>>>> +static void _eventfd_wqh_notify(struct eventfd_notifier *en)
>>>>> +{
>>>>> +	struct eventfd_ctx *ctx =3D container_of(en,
>>>>> +					       struct eventfd_ctx,
>>>>> +					       notifier);
>>>>> +
>>>>> +	if (waitqueue_active(&ctx->wqh))
>>>>> +		wake_up_poll(&ctx->wqh, POLLIN);
>>>>> +}
>>>>> +
>>>>> +static void _eventfd_notify(struct eventfd_ctx *ctx)
>>>>> +{
>>>>> +	struct eventfd_notifier *en;
>>>>> +	int idx;
>>>>> +
>>>>> +	idx =3D srcu_read_lock(&ctx->srcu);
>>>>> +
>>>>> +	/*
>>>>> +	 * The goal here is to allow the notification to be preemptible
>>>>> +	 * as often as possible.  We cannot achieve this with the basic
>>>>> +	 * wqh mechanism because it requires the wqh->lock.  Therefore
>>>>> +	 * we have an internal srcu list mechanism of which the wqh is
>>>>> +	 * a client.
>>>>> +	 *
>>>>> +	 * Not all paths will invoke this function in process context.
>>>>> +	 * Callers should check for suitable state before assuming they
>>>>> +	 * can sleep (such as with preemptible()).  Paul McKenney assures=

>>>>> +	 * me that srcu_read_lock is compatible with in-atomic, as long a=
s
>>>>> +	 * the code within the critical section is also compatible.
>>>>> +	 */
>>>>> +	list_for_each_entry_rcu(en, &ctx->nh, list)
>>>>> +		en->ops->signal(en);
>>>>> +
>>>>> +	srcu_read_unlock(&ctx->srcu, idx);
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Adds "n" to the eventfd counter "count". Returns "n" in case of=

>>>>>   * success, or a value lower then "n" in case of coutner overflow.=

>>>>>    =20
>>>>>        =20
>>>>>          =20
>>>> This is ugly, isn't it? With CONFIG_PREEMPT=3Dno preemptible() is al=
ways false.
>>>>
>>>> Further, to do useful things it might not be enough that you can sle=
ep:
>>>> with iofd you also want to access current task with e.g. copy from u=
ser.
>>>>
>>>> Here's an idea: let's pass a flag to ->signal, along the lines of
>>>> signal_is_task, that tells us that it is safe to use current, and ad=
d
>>>> eventfd_signal_task() which is the same as eventfd_signal but lets e=
veryone
>>>> know that it's safe to both sleep and use current->mm.
>>>>
>>>> Makes sense?
>>>>  =20
>>>>      =20
>>>>        =20
>>> It does make sense, yes.  What I am not clear on is how would eventfd=

>>> detect this state such as to populate such flags, and why cant the
>>> ->signal() CB do the same?
>>>
>>> Thanks Michael,
>>> -Greg
>>>
>>>    =20
>>>      =20
>> eventfd can't detect this state. But the callers know in what context =
they are.
>> So the *caller* of eventfd_signal_task makes sure of this: if you are =
in a task,
>> you can call eventfd_signal_task() if not, you must call eventfd_signa=
l.
>>
>>
>>  =20
>>    =20
> Hmm, this is an interesting idea, but I think it would be problematic i=
n
> real-world applications for the long-term.  For instance, the -rt tree
> and irq-threads .config option in the process of merging into mainline
> changes context types for established code.  Therefore, what might be
> "hardirq/softirq" logic today may execute in a kthread tomorrow.  I
> think its dangerous to try to solve the problem with caller provided
> info:  the caller may be ignorant of its true state.

Also, we need to consider that a process context can still be in-atomic
if the user did something like disabled interrupts, preemption, used a
spinlock, etc, before calling the eventfd_signal_task() function.=20
Perhaps we can put a stake in the ground that says you must not call
this from atomic context, but I still prefer just being able to detect
this from our state.

-Greg


--------------enig9D20F261EB862F1551BF1803
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.11 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAko3siQACgkQlOSOBdgZUxlHZQCaA5YLwQAwijQqWjBvVTRxyq//
yrUAn1OO5fidoKc8sNeeUmRujhmhiu8A
=xRBc
-----END PGP SIGNATURE-----

--------------enig9D20F261EB862F1551BF1803--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/