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

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

On 27.05.2021 20:48, Julien Grall wrote:
> 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 
> risk that port_is_valid() may be valid and then invalid because 
> d->valid_evtchns is decremented in evtchn_destroy().

While this is the case for other functions as well (and hence a
comment along the lines of what you ask for below should have
been in place already), I've added

 * While calling the function is okay without holding a suitable lock yet
 * (see the comment ahead of struct evtchn_port_ops for which ones those
 * are), for a dying domain it may start returning false at any point - see
 * evtchn_destroy(). This is not a fundamental problem though, as the
 * struct evtchn instance won't disappear (and will continue to hold valid
 * data) until final cleanup of the domain, at which point the domain itself
 * cannot be looked up anymore and hence calls here can't occur anymore in
 * the first place.


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

... ahead of port_is_valid() (and not, as I did intend originally,
in evtchn_close()). As far as the commit message goes, I'll have it
refer to the comment only.

I hope this satisfies the requests of both of you. I'll take the
liberty and retain your ack, Roger.




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