Re: [KVM-RFC PATCH 1/2] eventfd: add an explicit srcu based notifier interface

Gregory Haskins
2009-06-16T16:17:44Z

[-- Attachment #1: Type: text/plain, Size: 8836 bytes --]

Michael S. Tsirkin wrote:
> On Tue, Jun 16, 2009 at 11:20:18AM -0400, Gregory Haskins wrote:
>
>>>>> 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?
>>
>
> Yea. Sorry.
>

np. I knew what you meant ;)
>
>>>
>>>
>>>> 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
>>
>
> I think it's wrong to sleep for a long time while handling interrupts even if
> you technically can with threaded interrupts.

Well, this is somewhat of an orthogonal issue so I don't want to open
this can of worms per se.

But, one of the long-term goals of the threaded-irq construct is to
eliminate the need for the traditional "bh" contexts to do work. The
idea, of course, is that the threaded-irq context can do all of the work
traditionally shunted to tasklets/softirqs/workqueues directly, so why
bother switching contexts. So, the short answer is that its not
necessarily wrong to sleep or to do significant processing from a
threaded-irq.

In any case, threaded-irqs are just one example. I will try to
highlight others below.

> For that matter, I think you can
> sleep while within a spinlock if preempt is on
Yes, in fact the spinlocks are mutexes in this mode, so the locks
themselves can sleep.

> , but that does not mean you
> should - and I think you will get warnings in the log if you do. No?
>
>
Nope, sleeping is fine (voluntary or involuntary). The idea is its all
governed by priority, and priority-inheritance, and a scheduler that is
free to make fine-grained decisions (due to the broadly preemptible
kernel). But this is getting off-topic so I will stop.

>> (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.
>>
>
> This specific issue should just be addressed by making it possible
> to inject the interrupt from an atomic context.
>

I assume you mean
s/mutex_lock(&kvm->irq_lock)/spin_lock(&kvm->irq_lock)? If so, I agree
this is a good idea. TBH: I am more concerned about the iosignalfd path
w.r.t. the premptiblity of the callback. We can optimize the virtio-net
interface, for instance, once we make this ->signal() callback
preemptible. Having irqfd ->signal() preemptible is just a bonus, but
we could work around it by fixing irq_lock as well, I agree.

>
>> 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().
>>
>
> What, a path that is never invoked without threaded interrupts?
> Yes, I would say it currently looks like an over-optimization.
>

You are only seeing part of the picture though. This is a cascading
pattern.
>
>> 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.
>>
>
> Maybe I misunderstand what iosignalfd is - isn't it true that you get eventfd
> and kvm will signal that when guest performs io write to a specific
> address, and then drivers can get eventfd and poll it to detect
> these writes?
>

Correct.

> If yes, you have no way to know that the other end of eventfd
> is connected to kvm, so you don't know you can access current->mm.
>

Well, my intended use for them *does* know its connected to KVM.
Perhaps there will be others that do not in the future, but like I said
it could be addressed as those actually arise.

>
>> 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
>>
>
> But, it addresses the CONFIG_PREEMPT off case, which your design doesn't
> seem to.
>

Well, the problem is that it only addresses it partially in a limited
set of circumstances, and doesn't address the broader problem. I'll
give you an example:

(First off, lets assume that we are not going to have
eventfd_signal_task(), but rather eventfd_signal() with two option
flags: EVENTFD_SIGNAL_CANSLEEP, and EVENTFD_SIGNAL_CURRENTVALID

Today vbus-enet has a rx-thread and a tx-thread at least partially
because I need process-context to do the copy_to_user(other->mm) type
stuff (and we will need similar for our virtio-net backend as well). I
also utilize irqfd to do interrupt injection. Now, since I know that I
have kthread_context, I could modify vbus-enet to inject interrupts with
EVENTFD_SIGNAL_CANSLEEP set, and this is fine. I know that is safe.

However, the problem is above that! I would like to optimize out the
tx-thread to begin with with a similar "can_sleep()" pattern whenever
possible.

For instance, if the netif stack calls me to do a transmit and it
happens to be in a sleepable context, it would be nice to just skip
waking up the tx-thread. I would therefore just do the
copy_to_user(other->mm) + irqfd directly in the netif callback, thereby
skipping the context switch.

So the problem is a pattern that I would like to address generally
outside of just eventfd: "can I be sleepy"? If yes, do it now, if not
defer it.

So if the stack calls me in a preemptible state, I would like to detect
that and optimize my deferment mechanisms away. This isn't something
that happens readily today given the way the stacks locking and
softirq-net work, but its moving in that direction (for instance,
threaded softirqs).

This is why I am pushing for a run-time detection mechanism (like
can_sleep()), as opposed to something in the eventfd interface level
(like EVENTFD_SIGNAL_CANSLEEP). I think at least the CURRENTVALID idea
is a great one, I just don't have a current need for it. That said, I
do not have a problem with modifing eventfd to provide such information
if Davide et. al. ack it as well.

Does this all make sense?

-Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]