[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()



On 30.10.20 11:57, Julien Grall wrote:


On 30/10/2020 10:49, Jan Beulich wrote:
On 30.10.2020 11:38, Julien Grall wrote:
On 22/10/2020 17:17, Jan Beulich wrote:
On 22.10.2020 18:00, Roger Pau Monné wrote:
On Tue, Oct 20, 2020 at 04:10:09PM +0200, Jan Beulich wrote:
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -177,9 +177,16 @@ int evtchn_reset(struct domain *d, bool
    * Low-level event channel port ops.
    *
    * All hooks have to be called with a lock held which prevents the channel - * from changing state. This may be the domain event lock, the per-channel - * lock, or in the case of sending interdomain events also the other side's - * per-channel lock. Exceptions apply in certain cases for the PV shim.
+ * from changing state. This may be
+ * - the domain event lock,
+ * - the per-channel lock,
+ * - in the case of sending interdomain events the other side's per-channel
+ *   lock,
+ * - in the case of sending non-global vIRQ-s the per-vCPU virq_lock (in
+ *   combination with the ordering enforced through how the vCPU's
+ *   virq_to_evtchn[] gets updated),
+ * - in the case of sending global vIRQ-s vCPU 0's virq_lock.
+ * Exceptions apply in certain cases for the PV shim.

Having such a wide locking discipline looks dangerous to me, it's easy
to get things wrong without notice IMO.

It is effectively only describing how things are (or were before
XSA-343, getting restored here).

I agree with Roger here, the new/old locking discipline is dangerous and
it is only a matter of time before it will bite us again.

I think we should consider Juergen's series because the locking for the
event channel is easier to understand.

We should, yes. The one thing I'm a little uneasy with is the
new lock "variant" that gets introduced. Custom locking methods
also are a common source of problems (which isn't to say I see
any here).

I am also unease with a new lock "variant". However, this is the best proposal I have seen so far to unblock the issue.

I am open to other suggestion with simple locking discipline.

In theory my new lock variant could easily be replaced by a rwlock and
using the try-variant for the readers only. The disadvantage of that
approach would be a growth of struct evtchn.


Juergen



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.