These are grouped into a series largely because of their origin,
not so much because there are (heavy) dependencies among them.
The main change from v4 is the dropping of the two patches trying
to do away with the double event lock acquires in interdomain
channel handling. See also the individual patches.

1: use per-channel lock where possible
2: convert domain event lock to an r/w one
3: slightly defer lock acquire where possible
4: add helper for port_is_valid() + evtchn_from_port()
5: type adjustments
6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()

Jan

On 27.01.2021 09:13, Jan Beulich wrote:
> These are grouped into a series largely because of their origin,
> not so much because there are (heavy) dependencies among them.
> The main change from v4 is the dropping of the two patches trying
> to do away with the double event lock acquires in interdomain
> channel handling. See also the individual patches.
> 
> 1: use per-channel lock where possible
> 2: convert domain event lock to an r/w one
> 3: slightly defer lock acquire where possible
> 4: add helper for port_is_valid() + evtchn_from_port()
> 5: type adjustments
> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()

Only patch 4 here has got an ack so far - may I ask for clear feedback
as to at least some of these being acceptable (I can see the last one
being controversial, and if this was the only one left I probably
wouldn't even ping, despite thinking that it helps reduce unecessary
overhead).

Jan

On 21/04/2021 16:23, Jan Beulich wrote:
> On 27.01.2021 09:13, Jan Beulich wrote:
>> These are grouped into a series largely because of their origin,
>> not so much because there are (heavy) dependencies among them.
>> The main change from v4 is the dropping of the two patches trying
>> to do away with the double event lock acquires in interdomain
>> channel handling. See also the individual patches.
>>
>> 1: use per-channel lock where possible
>> 2: convert domain event lock to an r/w one
>> 3: slightly defer lock acquire where possible
>> 4: add helper for port_is_valid() + evtchn_from_port()
>> 5: type adjustments
>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
> 
> Only patch 4 here has got an ack so far - may I ask for clear feedback
> as to at least some of these being acceptable (I can see the last one
> being controversial, and if this was the only one left I probably
> wouldn't even ping, despite thinking that it helps reduce unecessary
> overhead).

I left feedback for the series one the previous version (see [1]). It 
would have been nice is if it was mentionned somewhere as this is still 
unresolved.

The locking in the event channel is already quite fragile (see recent 
XSAs, follow-up bugs...). Even if the pattern is used somewhere (as you 
suggested), I don't think it is good idea to pertain it.

To be clear, I am not saying I don't care about performance. Instead I 
am trying to find a trade off between code maintenability and 
performance. So far, I didn't see any data justifying that the extra 
performance is worth the risk of making a code more fragile.

Cheers,

[1] <3c393170-09f9-6d31-c227-b599f8769e35@xen.org>

> 
> Jan
> 

-- 
Julien Grall

On 21.04.2021 17:56, Julien Grall wrote:
> 
> 
> On 21/04/2021 16:23, Jan Beulich wrote:
>> On 27.01.2021 09:13, Jan Beulich wrote:
>>> These are grouped into a series largely because of their origin,
>>> not so much because there are (heavy) dependencies among them.
>>> The main change from v4 is the dropping of the two patches trying
>>> to do away with the double event lock acquires in interdomain
>>> channel handling. See also the individual patches.
>>>
>>> 1: use per-channel lock where possible
>>> 2: convert domain event lock to an r/w one
>>> 3: slightly defer lock acquire where possible
>>> 4: add helper for port_is_valid() + evtchn_from_port()
>>> 5: type adjustments
>>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
>>
>> Only patch 4 here has got an ack so far - may I ask for clear feedback
>> as to at least some of these being acceptable (I can see the last one
>> being controversial, and if this was the only one left I probably
>> wouldn't even ping, despite thinking that it helps reduce unecessary
>> overhead).
> 
> I left feedback for the series one the previous version (see [1]). It 
> would have been nice is if it was mentionned somewhere as this is still 
> unresolved.

I will admit I forgot about the controversy on patch 1. I did, however,
reply to your concerns. What didn't happen is the feedback from others
that you did ask for.

And of course there are 4 more patches here (one of them having an ack,
yes) which could do with feedback. I'm certainly willing, where possible,
to further re-order the series such that controversial changes are at its
end.

Jan

On Thu, Apr 22, 2021 at 10:53:05AM +0200, Jan Beulich wrote:
> On 21.04.2021 17:56, Julien Grall wrote:
> > 
> > 
> > On 21/04/2021 16:23, Jan Beulich wrote:
> >> On 27.01.2021 09:13, Jan Beulich wrote:
> >>> These are grouped into a series largely because of their origin,
> >>> not so much because there are (heavy) dependencies among them.
> >>> The main change from v4 is the dropping of the two patches trying
> >>> to do away with the double event lock acquires in interdomain
> >>> channel handling. See also the individual patches.
> >>>
> >>> 1: use per-channel lock where possible
> >>> 2: convert domain event lock to an r/w one
> >>> 3: slightly defer lock acquire where possible
> >>> 4: add helper for port_is_valid() + evtchn_from_port()
> >>> 5: type adjustments
> >>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
> >>
> >> Only patch 4 here has got an ack so far - may I ask for clear feedback
> >> as to at least some of these being acceptable (I can see the last one
> >> being controversial, and if this was the only one left I probably
> >> wouldn't even ping, despite thinking that it helps reduce unecessary
> >> overhead).
> > 
> > I left feedback for the series one the previous version (see [1]). It 
> > would have been nice is if it was mentionned somewhere as this is still 
> > unresolved.
> 
> I will admit I forgot about the controversy on patch 1. I did, however,
> reply to your concerns. What didn't happen is the feedback from others
> that you did ask for.
> 
> And of course there are 4 more patches here (one of them having an ack,
> yes) which could do with feedback. I'm certainly willing, where possible,
> to further re-order the series such that controversial changes are at its
> end.

I think it would easier to figure out whether the changes are correct
if we had some kind of documentation about what/how the per-domain
event_lock and the per-event locks are supposed to be used. I don't
seem to be able to find any comments regarding how they are to be
used.

Regarding the changes itself in patch 1 (which I think has caused part
of the controversy here), I'm unsure they are worth it because the
functions modified all seem to be non-performance critical:
evtchn_status, domain_dump_evtchn_info, flask_get_peer_sid.

So I would say that unless we have clear rules written down for what
the per-domain event_lock protects, I would be hesitant to change any
of the logic, specially for critical paths.

Thanks, Roger.

On 14.05.2021 17:29, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 10:53:05AM +0200, Jan Beulich wrote:
>> On 21.04.2021 17:56, Julien Grall wrote:
>>>
>>>
>>> On 21/04/2021 16:23, Jan Beulich wrote:
>>>> On 27.01.2021 09:13, Jan Beulich wrote:
>>>>> These are grouped into a series largely because of their origin,
>>>>> not so much because there are (heavy) dependencies among them.
>>>>> The main change from v4 is the dropping of the two patches trying
>>>>> to do away with the double event lock acquires in interdomain
>>>>> channel handling. See also the individual patches.
>>>>>
>>>>> 1: use per-channel lock where possible
>>>>> 2: convert domain event lock to an r/w one
>>>>> 3: slightly defer lock acquire where possible
>>>>> 4: add helper for port_is_valid() + evtchn_from_port()
>>>>> 5: type adjustments
>>>>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
>>>>
>>>> Only patch 4 here has got an ack so far - may I ask for clear feedback
>>>> as to at least some of these being acceptable (I can see the last one
>>>> being controversial, and if this was the only one left I probably
>>>> wouldn't even ping, despite thinking that it helps reduce unecessary
>>>> overhead).
>>>
>>> I left feedback for the series one the previous version (see [1]). It 
>>> would have been nice is if it was mentionned somewhere as this is still 
>>> unresolved.
>>
>> I will admit I forgot about the controversy on patch 1. I did, however,
>> reply to your concerns. What didn't happen is the feedback from others
>> that you did ask for.
>>
>> And of course there are 4 more patches here (one of them having an ack,
>> yes) which could do with feedback. I'm certainly willing, where possible,
>> to further re-order the series such that controversial changes are at its
>> end.
> 
> I think it would easier to figure out whether the changes are correct
> if we had some kind of documentation about what/how the per-domain
> event_lock and the per-event locks are supposed to be used. I don't
> seem to be able to find any comments regarding how they are to be
> used.

I think especially in pass-through code there are a number of cases
where the per-domain lock really is being abused, simply for being
available without much further thought. I'm not convinced documenting
such abuse is going to help the situation. Yet of course I can see
that having documentation would make review easier ...

> Regarding the changes itself in patch 1 (which I think has caused part
> of the controversy here), I'm unsure they are worth it because the
> functions modified all seem to be non-performance critical:
> evtchn_status, domain_dump_evtchn_info, flask_get_peer_sid.
> 
> So I would say that unless we have clear rules written down for what
> the per-domain event_lock protects, I would be hesitant to change any
> of the logic, specially for critical paths.

Okay, I'll drop patch 1 and also patch 6 for being overly controversial.
Some of the other patches still look worthwhile to me, though. I'll
also consider moving the spin->r/w lock conversion last in the series.

Jan