lkml.org 
[lkml]   [2009]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface
    On Tue, Jun 16, 2009 at 10:54:28AM -0400, Gregory Haskins wrote:
    > >>>>> +static void _eventfd_notify(struct eventfd_ctx *ctx)
    > >>>>> +{
    > >>>>> + struct eventfd_notifier *en;
    > >>>>> + int idx;
    > >>>>> +
    > >>>>> + idx = 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 as
    > >>>>> + * 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.
    > >>>>>
    > >>>>>
    > >>>>>
    > >>>> This is ugly, isn't it? With CONFIG_PREEMPT=no preemptible() is always false.
    > >>>>
    > >>>> Further, to do useful things it might not be enough that you can sleep:
    > >>>> with iofd you also want to access current task with e.g. copy from user.
    > >>>>
    > >>>> 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 add
    > >>>> eventfd_signal_task() which is the same as eventfd_signal but lets everyone
    > >>>> know that it's safe to both sleep and use current->mm.
    > >>>>
    > >>>> Makes sense?
    > >>>>
    > >>>>
    > >>>>
    > >>> 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
    > >>>
    > >>>
    > >>>
    > >> 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_signal.
    > >>
    > >>
    > >>
    > >>
    > > Hmm, this is an interesting idea, but I think it would be problematic in
    > > 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.
    > Perhaps we can put a stake in the ground that says you must not call
    > this from atomic context,

    That's the ticket.

    > but I still prefer just being able to detect
    > this from our state.
    >
    > -Greg
    >
    >

    --
    MST


    \
     
     \ /
      Last update: 2009-06-16 17:19    [W:0.031 / U:0.340 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site