Re: [PATCH 11/12] evtchn: convert vIRQ lock to an r/w one

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 02/10/2020 07:12, Jan Beulich wrote:
> On 01.10.2020 18:21, Julien Grall wrote:
> > On 30/09/2020 11:16, Jan Beulich wrote:
> > > On 30.09.2020 10:52, Paul Durrant wrote:
> > > > Looking again, given that both send_guest_vcpu_virq() and
> > > > send_guest_global_virq() (rightly) hold the evtchn lock before
> > > > calling evtchn_port_set_pending() I think you could do away with
> > > > the virq lock by adding checks in those functions to verify
> > > > evtchn->state == ECS_VIRQ and u.virq == virq after having
> > > > acquired the channel lock but before calling
> > > > evtchn_port_set_pending().
> > > I don't think so: The adjustment of v->virq_to_evtchn[] in
> > > evtchn_close() would then happen with just the domain's event
> > > lock held, which the sending paths don't use at all. The per-
> > > channel lock gets acquired in evtchn_close() a bit later only
> > > (and this lock can't possibly protect per-vCPU state).
> > >
> > > In fact I'm now getting puzzled by evtchn_bind_virq() updating
> > > this array with (just) the per-domain lock held. Since it's
> > > the last thing in the function, there's technically no strict
> > > need for acquiring the vIRQ lock,
> > Well, we at least need to prevent the compiler to tear the store/load.
> > If we don't use a lock, then we would should use ACCESS_ONCE() or
> > {read,write}_atomic() for all the usage.
> >
> > > but holding the event lock
> > > definitely doesn't help.
> > It helps because spin_unlock() and write_unlock() use the same barrier
> > (arch_lock_release_barrier()). So ...
> I'm having trouble making this part of your reply fit ...
>
> > > All that looks to be needed is the
> > > barrier implied from write_unlock().
> > No barrier should be necessary. Although, I would suggest to add a
> > comment explaining it.
> ... this. If we moved the update of v->virq_to_evtchn[] out of the
> locked region (as the lock doesn't protect anything anymore at that
> point), I think a barrier would need adding, such that the sending
> paths will observe the update by the time evtchn_bind_virq()
> returns (and hence sending of a respective vIRQ event can
> legitimately be expected to actually work). Or did you possibly
> just misunderstand what I wrote before? By putting in question the
> utility of holding the event lock, I implied the write could be
> moved out of the locked region ...
We are probably talking past each other... My point was that if we leave 
the write where it currently is, then we don't need an extra barrier 
because the spin_unlock() already contains the barrier we want.
Hence the suggestion to add a comment so a reader doesn't spend time 
wondering how this is safe...
Cheers,

--
Julien Grall