Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
> Hi Jan,
> 
> On 27/05/2021 12:28, Jan Beulich wrote:
> > port_is_valid() and evtchn_from_port() are fine to use without holding
> > any locks. Accordingly acquire the per-domain lock slightly later in
> > evtchn_close() and evtchn_bind_vcpu().
> 
> So I agree that port_is_valid() and evtchn_from_port() are fine to use
> without holding any locks in evtchn_bind_vcpu(). However, this is misleading
> to say there is no problem with evtchn_close().
> 
> evtchn_close() can be called with current != d and therefore, there is a

The only instances evtchn_close is called with current != d and the
domain could be unpaused is in free_xen_event_channel AFAICT.

> risk that port_is_valid() may be valid and then invalid because
> d->valid_evtchns is decremented in evtchn_destroy().

Hm, I guess you could indeed have parallel calls to
free_xen_event_channel and evtchn_destroy in a way that
free_xen_event_channel could race with valid_evtchns getting
decreased?

All callers of free_xen_event_channel are internal to the hypervisor,
so maybe with proper ordering of the operations this could be avoided,
albeit it's not easy to assert.

> Thankfully the memory is still there. So the current code is okayish and I
> could reluctantly accept this behavior to be spread. However, I don't think
> this should be left uncommented in both the code (maybe on top of
> port_is_valid()?) and the commit message.

Indeed, I think we need some expansion of the comment in port_is_valid
to clarify all this. I'm not sure I understand it properly myself when
it's fine to use port_is_valid without holding the per domain event
lock.

evtchn_close shouldn't be a very common operation anyway, so it
holding the per domain lock a bit longer doesn't seem that critical to
me anyway.

Thanks, Roger.