Mailing List Archive

[PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
The per-vCPU virq_lock, which is being held anyway, together with there
not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[]
is zero, provide sufficient guarantees. Undo the lock addition done for
XSA-343 (commit e045199c7c9c "evtchn: address races with
evtchn_reset()"). Update the description next to struct evtchn_port_ops
accordingly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -794,7 +794,6 @@ void send_guest_vcpu_virq(struct vcpu *v
unsigned long flags;
int port;
struct domain *d;
- struct evtchn *chn;

ASSERT(!virq_is_global(virq));

@@ -805,10 +804,7 @@ void send_guest_vcpu_virq(struct vcpu *v
goto out;

d = v->domain;
- chn = evtchn_from_port(d, port);
- spin_lock(&chn->lock);
- evtchn_port_set_pending(d, v->vcpu_id, chn);
- spin_unlock(&chn->lock);
+ evtchn_port_set_pending(d, v->vcpu_id, evtchn_from_port(d, port));

out:
spin_unlock_irqrestore(&v->virq_lock, flags);
@@ -837,9 +833,7 @@ void send_guest_global_virq(struct domai
goto out;

chn = evtchn_from_port(d, port);
- spin_lock(&chn->lock);
evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
- spin_unlock(&chn->lock);

out:
spin_unlock_irqrestore(&v->virq_lock, flags);
--- 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.
*/
struct evtchn_port_ops {
void (*init)(struct domain *d, struct evtchn *evtchn);
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On Tue, Oct 20, 2020 at 04:10:09PM +0200, Jan Beulich wrote:
> The per-vCPU virq_lock, which is being held anyway, together with there
> not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[]
> is zero, provide sufficient guarantees.

This is also fine because closing the event channel will be done with
the VIRQ hold held also AFAICT.

> Undo the lock addition done for
> XSA-343 (commit e045199c7c9c "evtchn: address races with
> evtchn_reset()"). Update the description next to struct evtchn_port_ops
> accordingly.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -794,7 +794,6 @@ void send_guest_vcpu_virq(struct vcpu *v
> unsigned long flags;
> int port;
> struct domain *d;
> - struct evtchn *chn;
>
> ASSERT(!virq_is_global(virq));
>
> @@ -805,10 +804,7 @@ void send_guest_vcpu_virq(struct vcpu *v
> goto out;
>
> d = v->domain;
> - chn = evtchn_from_port(d, port);
> - spin_lock(&chn->lock);
> - evtchn_port_set_pending(d, v->vcpu_id, chn);
> - spin_unlock(&chn->lock);
> + evtchn_port_set_pending(d, v->vcpu_id, evtchn_from_port(d, port));
>
> out:
> spin_unlock_irqrestore(&v->virq_lock, flags);
> @@ -837,9 +833,7 @@ void send_guest_global_virq(struct domai
> goto out;
>
> chn = evtchn_from_port(d, port);
> - spin_lock(&chn->lock);
> evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
> - spin_unlock(&chn->lock);
>
> out:
> spin_unlock_irqrestore(&v->virq_lock, flags);
> --- 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.

Maybe we could add an assert to that effect in
evtchn_port_set_pending in order to make sure callers follow the
discipline?

Roger.
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 22.10.2020 18:00, Roger Pau Monné wrote:
> On Tue, Oct 20, 2020 at 04:10:09PM +0200, Jan Beulich wrote:
>> The per-vCPU virq_lock, which is being held anyway, together with there
>> not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[]
>> is zero, provide sufficient guarantees.
>
> This is also fine because closing the event channel will be done with
> the VIRQ hold held also AFAICT.

Right, I'm not going into these details (or else binding would also
need mentioning) here, as the code comment update should sufficiently
cover it. Hence just saying "sufficient guarantees".

>> --- 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).

> Maybe we could add an assert to that effect in
> evtchn_port_set_pending in order to make sure callers follow the
> discipline?

Would be nice, but (a) see the last sentence of the comment still
in context above and (b) it shouldn't be just set_pending(). The
comment starts with "All hooks ..." after all.

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

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:
>>> The per-vCPU virq_lock, which is being held anyway, together with there
>>> not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[]
>>> is zero, provide sufficient guarantees.
>>
>> This is also fine because closing the event channel will be done with
>> the VIRQ hold held also AFAICT.
>
> Right, I'm not going into these details (or else binding would also
> need mentioning) here, as the code comment update should sufficiently
> cover it. Hence just saying "sufficient guarantees".
>
>>> --- 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.

With his series in place, this patch will become unecessary.

Cheers,

--
Julien Grall
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
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).

> With his series in place, this patch will become unecessary.

It'll become less important, but not pointless - any unnecessary
locking would better be removed imo.

I'd also like to note that the non-straightforward locking rules
wouldn't really change with his series; the benefit there really
is the dropping of the need for IRQ-safe locking.

Jan
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
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.

>
>> With his series in place, this patch will become unecessary.
>
> It'll become less important, but not pointless - any unnecessary
> locking would better be removed imo.

They may be unnecessary today but if tomorrow someone decide to rework
the other lock, then you are just re-opening a security hole.

IHMO, having a sane locking system is far more important than removing
locking that look "unnecessary".

>
> I'd also like to note that the non-straightforward locking rules
> wouldn't really change with his series; the benefit there really
> is the dropping of the need for IRQ-safe locking.

Well, it is at least going towards that...

Cheers,

>
> Jan
>

--
Julien Grall
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
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
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.2020 12:15, Jürgen Groß wrote:
> 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.

Well, only until we would add check_lock() there, which I think
we should really have (not just on the slow paths, thanks to
the use of spin_lock() there). The read-vs-write properties
you're utilizing aren't applicable in the general case afaict,
and hence such checking would get in the way.

> The disadvantage of that approach would be a growth of struct evtchn.

Wasn't it you who had pointed out to me the aligned(64) attribute
on the struct (in a different context), which afaict would subsume
any possible growth?

Jan
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.20 12:55, Jan Beulich wrote:
> On 30.10.2020 12:15, Jürgen Groß wrote:
>> 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.
>
> Well, only until we would add check_lock() there, which I think
> we should really have (not just on the slow paths, thanks to
> the use of spin_lock() there). The read-vs-write properties
> you're utilizing aren't applicable in the general case afaict,
> and hence such checking would get in the way.

No, I don't think so.

As long as there is no read_lock() user with interrupts off we should be
fine. read_trylock() is no problem as it can't block.

>
>> The disadvantage of that approach would be a growth of struct evtchn.
>
> Wasn't it you who had pointed out to me the aligned(64) attribute
> on the struct (in a different context), which afaict would subsume
> any possible growth?

Oh, indeed.

The growth would be 8 bytes, leading to a max of 56 bytes then.


Juergen
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.2020 13:27, Jürgen Groß wrote:
> On 30.10.20 12:55, Jan Beulich wrote:
>> On 30.10.2020 12:15, Jürgen Groß wrote:
>>> 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:
>>>>>> 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.
>>
>> Well, only until we would add check_lock() there, which I think
>> we should really have (not just on the slow paths, thanks to
>> the use of spin_lock() there). The read-vs-write properties
>> you're utilizing aren't applicable in the general case afaict,
>> and hence such checking would get in the way.
>
> No, I don't think so.
>
> As long as there is no read_lock() user with interrupts off we should be
> fine. read_trylock() is no problem as it can't block.

How would check_lock() notice the difference? It would be all the
same for read and write acquires of the lock, I think, and hence
it would still get unhappy about uses from IRQ paths afaict.

Jan
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.20 13:52, Jan Beulich wrote:
> On 30.10.2020 13:27, Jürgen Groß wrote:
>> On 30.10.20 12:55, Jan Beulich wrote:
>>> On 30.10.2020 12:15, Jürgen Groß wrote:
>>>> 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:
>>>>>>> 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.
>>>
>>> Well, only until we would add check_lock() there, which I think
>>> we should really have (not just on the slow paths, thanks to
>>> the use of spin_lock() there). The read-vs-write properties
>>> you're utilizing aren't applicable in the general case afaict,
>>> and hence such checking would get in the way.
>>
>> No, I don't think so.
>>
>> As long as there is no read_lock() user with interrupts off we should be
>> fine. read_trylock() is no problem as it can't block.
>
> How would check_lock() notice the difference? It would be all the
> same for read and write acquires of the lock, I think, and hence
> it would still get unhappy about uses from IRQ paths afaict.

check_lock() isn't applicable here, at least not without modification.

I think our spinlock implementation is wrong in this regard in case a
lock is entered via spin_trylock(), BTW. Using spin_trylock() with
interrupts off for a lock normally taken with interrupts enabled is
perfectly fine IMO.


Juergen
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.2020 14:02, Jürgen Groß wrote:
> On 30.10.20 13:52, Jan Beulich wrote:
>> On 30.10.2020 13:27, Jürgen Groß wrote:
>>> On 30.10.20 12:55, Jan Beulich wrote:
>>>> On 30.10.2020 12:15, Jürgen Groß wrote:
>>>>> 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:
>>>>>>>> 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.
>>>>
>>>> Well, only until we would add check_lock() there, which I think
>>>> we should really have (not just on the slow paths, thanks to
>>>> the use of spin_lock() there). The read-vs-write properties
>>>> you're utilizing aren't applicable in the general case afaict,
>>>> and hence such checking would get in the way.
>>>
>>> No, I don't think so.
>>>
>>> As long as there is no read_lock() user with interrupts off we should be
>>> fine. read_trylock() is no problem as it can't block.
>>
>> How would check_lock() notice the difference? It would be all the
>> same for read and write acquires of the lock, I think, and hence
>> it would still get unhappy about uses from IRQ paths afaict.
>
> check_lock() isn't applicable here, at least not without modification.
>
> I think our spinlock implementation is wrong in this regard in case a
> lock is entered via spin_trylock(), BTW. Using spin_trylock() with
> interrupts off for a lock normally taken with interrupts enabled is
> perfectly fine IMO.

Hmm, I think you're right, in which I case guess we ought to relax
this.

Jan
Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() [ In reply to ]
On 30.10.20 14:38, Jan Beulich wrote:
> On 30.10.2020 14:02, Jürgen Groß wrote:
>> On 30.10.20 13:52, Jan Beulich wrote:
>>> On 30.10.2020 13:27, Jürgen Groß wrote:
>>>> On 30.10.20 12:55, Jan Beulich wrote:
>>>>> On 30.10.2020 12:15, Jürgen Groß wrote:
>>>>>> 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:
>>>>>>>>> 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.
>>>>>
>>>>> Well, only until we would add check_lock() there, which I think
>>>>> we should really have (not just on the slow paths, thanks to
>>>>> the use of spin_lock() there). The read-vs-write properties
>>>>> you're utilizing aren't applicable in the general case afaict,
>>>>> and hence such checking would get in the way.
>>>>
>>>> No, I don't think so.
>>>>
>>>> As long as there is no read_lock() user with interrupts off we should be
>>>> fine. read_trylock() is no problem as it can't block.
>>>
>>> How would check_lock() notice the difference? It would be all the
>>> same for read and write acquires of the lock, I think, and hence
>>> it would still get unhappy about uses from IRQ paths afaict.
>>
>> check_lock() isn't applicable here, at least not without modification.
>>
>> I think our spinlock implementation is wrong in this regard in case a
>> lock is entered via spin_trylock(), BTW. Using spin_trylock() with
>> interrupts off for a lock normally taken with interrupts enabled is
>> perfectly fine IMO.
>
> Hmm, I think you're right, in which I case guess we ought to relax
> this.

Just writing a patch. :-)

And one for adding check_lock() to rwlocks, too.


Juergen