Re: Ping: [PATCH v5 0/6] evtchn: (not so) recent XSAs follow-on

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.