Michael S. Tsirkin wrote:
> On Tue, Jun 16, 2009 at 10:48:27AM -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.
>>
>
> That's OK, it's always safe to call eventfd_signal: eventfd_signal_task is
> just
> an optimization. I think everyone not in the context of a system call or
> vmexit
> can just call eventfd_signal_task.
>
^^^^^^^^^^^^^^^^^^^^I assume you meant s/eventfd_signal_task/eventfd_signal there? > >> I >> think its dangerous to try to solve the problem with caller provided >> info: the caller may be ignorant of its true state. >> > > I assume this wasn't clear enough: the idea is that you only > calls eventfd_signal_task if you know you are on a systemcall path. > If you are ignorant of the state, call eventfd_signal. > Well, its not a matter of correctness. Its more for optimal performance. If I have PCI pass-though injecting interrupts from hardirq in mainline, clearly eventfd_signal() is proper. In -rt, the hardirq is transparently converted to a kthread, so technically eventfd_signal_task() would work (at least for the can_sleep() part, not for current->mm per se). But in this case, the PCI logic would not know it was converted to a kthread. It all happens transparently in the low-level code and the pci code is unmodified. In this case, your proposal would have the passthrough path invoking irqfd with eventfd_signal(). It would therefore still shunt to a workqueue to inject the interrupt, even though it would have been perfectly fine to just inject it directly because taking mutex_lock(&kvm->irq_lock) is legal. Perhaps I am over-optimizing, but this is the scenario I am concerned about and what I was trying to address with preemptible()/can_sleep(). I think your idea is a good one to address the current->mm portion. It would only ever be safe to access the MM context from syscall/vmexit context, as you point out. Therefore, I see no problem with implementing something like iosignalfd with eventfd_signal_task(). But accessing current->mm is only a subset of the use-cases. The other use-cases would include the ability to sleep, and possibly the ability to address other->mm. For these latter cases, I really only need the "can_sleep()" behavior, not the full blown "can_access_current_mm()". Additionally, the eventfd_signal_task() data at least for iosignalfd is superfluous: I already know that I can access current->mm by virtue of the design. So since I cannot use it accurately for the hardirq/threaded-irq type case, and I don't actually need it for the iosignalfd case, I am not sure its the right direction (at least for now). I do think it might have merit for syscal/vmexit uses outside of iosignalfd, but I do not see a current use-case for it so perhaps it can wait until one arises. -Greg > >> IMO, the ideal >> solution needs to be something we can detect at run-time. >> >> Thanks Michael, >> -Greg >> >> > > >
signature.asc
Description: OpenPGP digital signature
