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
>>
>>
>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
