Re: [PATCH v2 5/8] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()

[Jürgen Groß <jgross@xxxxxxxx>]

On 30.10.20 14:38, Jan Beulich wrote:
> On 30.10.2020 14:02, Jürgen Groß wrote:
> > On 30.10.20 13:52, Jan Beulich wrote:
> > > On 30.10.2020 13:27, Jürgen Groß wrote:
> > > > On 30.10.20 12:55, Jan Beulich wrote:
> > > > > On 30.10.2020 12:15, Jürgen Groß wrote:
> > > > > > On 30.10.20 11:57, Julien Grall wrote:
> > > > > > > On 30/10/2020 10:49, Jan Beulich wrote:
> > > > > > > > On 30.10.2020 11:38, Julien Grall wrote:
> > > > > > > > > I think we should consider Juergen's series because the locking for the
> > > > > > > > > event channel is easier to understand.
> > > > > > > > We should, yes. The one thing I'm a little uneasy with is the
> > > > > > > > new lock "variant" that gets introduced. Custom locking methods
> > > > > > > > also are a common source of problems (which isn't to say I see
> > > > > > > > any here).
> > > > > > > I am also unease with a new lock "variant". However, this is the best
> > > > > > > proposal I have seen so far to unblock the issue.
> > > > > > >
> > > > > > > I am open to other suggestion with simple locking discipline.
> > > > > > In theory my new lock variant could easily be replaced by a rwlock and
> > > > > > using the try-variant for the readers only.
> > > > > Well, only until we would add check_lock() there, which I think
> > > > > we should really have (not just on the slow paths, thanks to
> > > > > the use of spin_lock() there). The read-vs-write properties
> > > > > you're utilizing aren't applicable in the general case afaict,
> > > > > and hence such checking would get in the way.
> > > > No, I don't think so.
> > > >
> > > > As long as there is no read_lock() user with interrupts off we should be
> > > > fine. read_trylock() is no problem as it can't block.
> > > How would check_lock() notice the difference? It would be all the
> > > same for read and write acquires of the lock, I think, and hence
> > > it would still get unhappy about uses from IRQ paths afaict.
> > check_lock() isn't applicable here, at least not without modification.
> >
> > I think our spinlock implementation is wrong in this regard in case a
> > lock is entered via spin_trylock(), BTW. Using spin_trylock() with
> > interrupts off for a lock normally taken with interrupts enabled is
> > perfectly fine IMO.
> Hmm, I think you're right, in which I case guess we ought to relax
> this.
Just writing a patch. :-)

And one for adding check_lock() to rwlocks, too.


Juergen