Re: [Xen-devel] [PATCH] x86/flush: use APIC ALLBUT destination shorthand when possible

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH] x86/flush: use APIC ALLBUT destination shorthand when possible
Date: Wed, 8 Jan 2020 14:30:57 +0100	[thread overview]
Message-ID: <20200108132819.GO11756@Air-de-Roger> (raw)
In-Reply-To: <57fe475e-c102-19a0-c2dd-8382046f1907@suse.com>

On Fri, Jan 03, 2020 at 01:55:51PM +0100, Jan Beulich wrote:
> On 03.01.2020 13:34, Roger Pau Monné wrote:
> > On Fri, Jan 03, 2020 at 01:08:20PM +0100, Jan Beulich wrote:
> >> On 24.12.2019 13:44, Roger Pau Monne wrote:
> >> Further a question on lock nesting: Since the commit message
> >> doesn't say anything in this regard, did you check there are no
> >> TLB flush invocations with the get_cpu_maps() lock held?
> > 
> > The CPU maps lock is a recursive one, so it should be fine to attempt
> > a TLB flush with the lock already held.
> 
> When already held by the same CPU - sure. It being a recursive
> one (which I paid attention to when writing my earlier reply)
> doesn't make it (together with any other one) immune against
> ABBA deadlocks, though.

There's no possibility of a deadlock here because get_cpu_maps does a
trylock, so if another cpu is holding the lock the flush will just
fallback to not using the shorthand.

> >> Even if
> >> you did and even if there are none, I think the function should
> >> then get a comment attached to the effect of this lock order
> >> inversion risk. (For example, it isn't obvious to me that no user
> >> of stop_machine() would ever want to do any kind of TLB flushing.)
> >>
> >> Overall I wonder whether your goal couldn't be achieved without
> >> the extra locking and without the special conditions.
> > 
> > Hm, this so far has worked fine on all the boxes that I've tried.
> > I'm happy to change it to a simpler approach, but I think the
> > conditions and locking are required for this to work correctly.
> 
> Which might then indicate said pre-existing use of cpu_online_map
> to be a (latent?) problem.

Hm, maybe it could be a problem when offlining a CPU. I assume this is
not an issue so far because there are no global TLB flushes with a
mask contaning a CPU that is being offlined.

Regarding the patch itself, do you think the shorthand logic should be
added to send_IPI_mask, or would you rather only use the shorthand for
the TLB flushes?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel