irqfd enhancements

Certain GSI's support lockless injecton, but we have no way to detect which ones at the GSI level. Knowledge of this attribute will be useful later in the series so that we can optimize irqfd injection paths for cases where we know the code will not sleep. Therefore, we provide an API to query a specific GSI.

Signed-off-by Gregory Haskins <[email protected]>

---

 include/linux/kvm_host.h |    2 ++
 virt/kvm/irq_comm.c      |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

include/linux/kvm_host.h +2 -0

index bd5a616..93393a4 100644
@@ -119,6 +119,7 @@ struct kvm_memory_slot {
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
 	u32 type;
+	bool lockless;
 	int (*set)(struct kvm_kernel_irq_routing_entry *e,
 		   struct kvm *kvm, int irq_source_id, int level);
 	union {

@@ -417,6 +418,7 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_irq_check_lockless(struct kvm *kvm, u32 irq);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

virt/kvm/irq_comm.c +2 -0

index 00c68d2..04f0134 100644
@@ -174,6 +174,23 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
 	return ret;
 }
 
+int kvm_irq_check_lockless(struct kvm *kvm, u32 irq)
+{

virt/kvm/irq_comm.c +7 -1

+	rcu_read_unlock();
+
+	return ret;
+}
+
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;

@@ -314,6 +331,7 @@ static int setup_routing_entry(struct kvm_irq_routing_table *rt,
 
 	e->gsi = ue->gsi;
 	e->type = ue->type;
+	e->lockless = false;
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		delta = 0;

@@ -342,6 +360,7 @@ static int setup_routing_entry(struct kvm_irq_routing_table *rt,
 		e->msi.address_lo = ue->u.msi.address_lo;
 		e->msi.address_hi = ue->u.msi.address_hi;
 		e->msi.data = ue->u.msi.data;
+		e->lockless = true;
 		break;
 	default:
 		goto out;
 
--

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

Certain GSI's support lockless injecton, but we have no way to detect which ones at the GSI level. Knowledge of this attribute will be useful later in the series so that we can optimize irqfd injection paths for cases where we know the code will not sleep. Therefore, we provide an API to query a specific GSI.

+int kvm_irq_check_lockless(struct kvm *kvm, u32 irq)

virt/kvm/irq_comm.c +5 -0

+	rcu_read_unlock();
+
+	return ret;
+}
+

Quoted 7 lines · Gregory Haskins

Certain GSI's support lockless injecton, but we have no way to detect which ones at the GSI level. Knowledge of this attribute will be useful later in the series so that we can optimize irqfd injection paths for cases where we know the code will not sleep. Therefore, we provide an API to query a specific GSI.

Instead of a lockless attribute, how about a ->set_atomic() method. For msi this can be the same as ->set(), for non-msi it can be a function that schedules the work (which will eventually call ->set()).

The benefit is that we make a decision only once, when preparing the routing entry, and install that decision in the routing entry instead of making it again and again later.

Quoted 3 lines

+int kvm_irq_check_lockless(struct kvm *kvm, u32 irq)

bool kvm_irq_check_lockless(...)

Quoted 12 lines · Avi Kivity

Quoted 7 lines · Gregory Haskins

Certain GSI's support lockless injecton, but we have no way to detect which ones at the GSI level. Knowledge of this attribute will be useful later in the series so that we can optimize irqfd injection paths for cases where we know the code will not sleep. Therefore, we provide an API to query a specific GSI.

Instead of a lockless attribute, how about a ->set_atomic() method. For msi this can be the same as ->set(), for non-msi it can be a function that schedules the work (which will eventually call ->set()).

Quoted 3 lines

The benefit is that we make a decision only once, when preparing the routing entry, and install that decision in the routing entry instead of making it again and again later.

Yeah, I like this idea. I think we can also get rid of the custom workqueue if we do this as well, TBD.

Quoted 8 lines

Quoted 8 lines

Instead of a lockless attribute, how about a ->set_atomic() method. For msi this can be the same as ->set(), for non-msi it can be a function that schedules the work (which will eventually call ->set()).

The benefit is that we make a decision only once, when preparing the routing entry, and install that decision in the routing entry instead of making it again and again later.

Quoted 3 lines

Yeah, I like this idea. I think we can also get rid of the custom workqueue if we do this as well, TBD.

So I looked into this. It isn't straight forward because you need to retain some kind of state across the deferment on a per-request basis (not per-GSI). Today, this state is neatly tracked into the irqfd object itself (e.g. it knows to toggle the GSI).

So while generalizing this perhaps makes sense at some point, especially if irqfd-like interfaces get added, it probably doesn't make a ton of sense to expend energy on it ATM. It is basically a generalization of the irqfd deferrment code. Lets just wait until we have a user beyond irqfd for now. Sound acceptable?

In the meantime, I found a bug in the irq_routing code, so I will submit a v3 with this fix, as well as a few other things I improved in the v2 series.

Quoted 10 lines · Gregory Haskins

Quoted 10 lines

Quoted 10 lines

Instead of a lockless attribute, how about a ->set_atomic() method. For
msi this can be the same as ->set(), for non-msi it can be a function that schedules the work (which will eventually call ->set()).

The benefit is that we make a decision only once, when preparing the routing entry, and install that decision in the routing entry instead of
making it again and again later.

Quoted 8 lines

Quoted 3 lines

Yeah, I like this idea. I think we can also get rid of the custom workqueue if we do this as well, TBD.

So I looked into this. It isn't straight forward because you need to retain some kind of state across the deferment on a per-request basis (not per-GSI). Today, this state is neatly tracked into the irqfd object itself (e.g. it knows to toggle the GSI).

Yes, and it also contains the work_struct.

What if we make the work_struct (and any additional state) part of the set_atomic() argument list? Does it simplify things?

Quoted 7 lines

So while generalizing this perhaps makes sense at some point, especially if irqfd-like interfaces get added, it probably doesn't make a ton of sense to expend energy on it ATM. It is basically a generalization of the irqfd deferrment code. Lets just wait until we have a user beyond irqfd for now. Sound acceptable?

I'll look at v3, but would really like to disentangle this.

Quoted 5 lines

Yes, and it also contains the work_struct.

What if we make the work_struct (and any additional state) part of the set_atomic() argument list? Does it simplify things?

Hmmm, that might not, but we could do a kmalloc(GFP_ATOMIC) for such parameters. Considering this is just a safety net, perhaps this would work fine.

Quoted 7 lines

Quoted 7 lines

So while generalizing this perhaps makes sense at some point, especially if irqfd-like interfaces get added, it probably doesn't make a ton of sense to expend energy on it ATM. It is basically a generalization of the irqfd deferrment code. Lets just wait until we have a user beyond irqfd for now. Sound acceptable?

Quoted 2 lines

I'll look at v3, but would really like to disentangle this.

Ok, I will see what I can do. I need at least a v4 to get rid of the dependency on the now defunct v3:1/3 patch per yesterdays discussion.

Quoted 5 lines · Gregory Haskins

Quoted 5 lines

Yes, and it also contains the work_struct.

What if we make the work_struct (and any additional state) part of the set_atomic() argument list? Does it simplify things?

Quoted 4 lines

Hmmm, that might not, but we could do a kmalloc(GFP_ATOMIC) for such parameters. Considering this is just a safety net, perhaps this would work fine.

Can't you simply pass the same work_struct from irqfd as we use now?

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

So while generalizing this perhaps makes sense at some point, especially
if irqfd-like interfaces get added, it probably doesn't make a ton of sense to expend energy on it ATM. It is basically a generalization of the irqfd deferrment code. Lets just wait until we have a user beyond irqfd for now. Sound acceptable?

Quoted 5 lines

Quoted 2 lines

I'll look at v3, but would really like to disentangle this.

Ok, I will see what I can do. I need at least a v4 to get rid of the dependency on the now defunct v3:1/3 patch per yesterdays discussion.

There's another alternative - make ioapic and pic irq-safe by switching irq locking to spinlocks and using spin_lock_irqsave().

I've long opposed this since the ioapic loops on all vcpus when injecting some irqs and this will increase irqoff times with large guests. But we don't have large guests now, and we need irq-safe injection in three places now:

- irqfd
- pit - we now signal vcpu0 to handle the injection, but this has its problems
- device assignment

so it may be better to have irq-safe injection, and deal with the loop later (would be good to have an idea how exactly).