On 17.04.20 14:26, Jan Beulich wrote:
On 17.04.2020 13:54, Juergen Gross wrote:
@@ -603,6 +611,7 @@ static void __unbind_from_irq(unsigned int irq)
{
evtchn_port_t evtchn = evtchn_from_irq(irq);
struct irq_info *info = irq_get_handler_data(irq);
+ unsigned long flags;
if (info->refcnt > 0) {
info->refcnt--;
@@ -610,8 +619,10 @@ static void __unbind_from_irq(unsigned int irq)
return;
}
+ write_lock_irqsave(&evtchn_rwlock, flags);
This placement looks odd - it doesn't guard the earlier if()
(i.e. isn't covering as wide a scope as one might expect)
but also isn't inside ...
if (VALID_EVTCHN(evtchn)) {
... this if(), which would be the minimal locked region needed).
Right, I can move the lock inside this if ().
While you have a comment at the declaration of the lock, I'd
recommend to have another one here clarifying that the less
than expected locked region is because the lock is to guard
only against removal, not also against other races (which I
assume are taken care of by other means).
Indeed, there is mutex_lock(&irq_mapping_update_lock) around all
invocations of __unbind_from_irq().
- unsigned int cpu = cpu_from_irq(irq);
+ unsigned int cpu = cpu_from_irq(irq);;
Stray and unwanted change?
Yes.
@@ -629,6 +640,8 @@ static void __unbind_from_irq(unsigned int irq)
xen_irq_info_cleanup(info);
}
+ write_unlock_irqrestore(&evtchn_rwlock, flags);
+
xen_free_irq(irq);
}
@@ -1219,6 +1232,8 @@ static void __xen_evtchn_do_upcall(void)
struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
int cpu = smp_processor_id();
+ read_lock(&evtchn_rwlock);
+
do {
vcpu_info->evtchn_upcall_pending = 0;
@@ -1229,6 +1244,8 @@ static void __xen_evtchn_do_upcall(void)
virt_rmb(); /* Hypervisor can set upcall pending. */
} while (vcpu_info->evtchn_upcall_pending);
+
+ read_unlock(&evtchn_rwlock);
}
So you guard against removal, but not against insertion - is
insertion done in a way that partial setup is never a problem?
(Looking at in particular the FIFO code I can't easily
convince myself this is the case.)
The IRQ related to the event channel is enabled only after the
event channel is setup completely. This is done by request_irq()
or similar calls (e.g. request_threaded_irq()).
I'll add a comment explaining that.
Juergen
