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

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

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

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


Julien Grall



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