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

Re: [PATCH] x86/flushtlb: remove flush_area check on system state


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 23 May 2022 16:37:31 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=w2ACdcgDh55CBtYDnC2JEEp/kSVONdmEgJM7SZUQMlo=; b=K6gw/5zVRHR87B32M8p94aEmjn5bQz6se99cGN+foUFIIpBafndOgUs0DNH2aXLQNUVefUxlqDDIMatRhL1Nj90itHHoSbw/3FP2qZDw9XW9nS2IPECJXLMOoW7g1CBffE8lI4scDLyZX9mfKrIEOWLv4VixAn086z86ODERE70vMVf4aMKRB1AYOc9UQtl9E6Psmne9u6vKbEAjZ3g6wvDgsTMCv0w5pFtEmzHBYyI9S76tNrh7FGuutzi3kzUsf92alGqgh6y9trbBewPAhtfQgIFmUYWJu7uuhTNVyAls1iB2gbqMuFZekX6HyDQrtVuCqKXmYZZfBljKPflUcw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gWuThzWFEo55aF/dx5FzZ4jPUjxy+WOy8hnRSyUP4pEvrYTJjIYmxumb2gxLIPOouN2mcH3+LhuMvpI/mAkjEIx1eJHiqm+X7CRWf2CqE5AmZ36D/WWgxrcc5icfkpQ9kpGpMdp2a4U+hPdLwFgovG9gEOEs/7cXELXzIyUleaOIyPQ1abbbM6+n8LOE5ZbGd6Ot7PCobf45HvZfRm5A1LIWdFm+ajLN3WDaNIK0+SYA/T0OGD/R4hqYmqQ5ZJjVcb6jEcnK5lBhzQ7Y2eObvDRWgZwI4Rg9bdvlXMte+rtCxaP0Nbxx3Mv5hlBqaKfMXAcLA4VD3P9ce5wLpwshtA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 23 May 2022 14:38:08 +0000
  • Ironport-data: A9a23:K7wCVKnBEZJKiHiP2efxMMHo5gz0J0RdPkR7XQ2eYbSJt1+Wr1Gzt xIcUD+HPvnbNGD3fdFxPYqy8UhSv5HVn4BhHAZk/yg2FyMWpZLJC+rCIxarNUt+DCFioGGLT Sk6QoOdRCzhZiaE/n9BCpC48T8kk/vgqoPUUIYoAAgoLeNfYHpn2EsLd9IR2NYy24DkWVjV4 LsenuWEULOb828sWo4rw/rrRCNH5JwebxtB4zTSzdgS1LPvvyF94KA3fMldHFOhKmVgJcaoR v6r8V2M1jixEyHBqD+Suu2TnkUiGtY+NOUV45Zcc/DKbhNq/kTe3kunXRa1hIg+ZzihxrhMJ NtxWZOYERYmHP2VisIhTyJfCixvJr0Y05D7CC3q2SCT5xWun3rE5dxLVRtzGLJCv+F9DCdJ6 OASLy0LYlabneWqzbmnS+5qwMM+MM3sO4BZsXZlpd3bJa9+HdafHOOUu5kEhF/chegXdRraT 9AeZjd1KgzJfjVEO0sNCYJ4l+Ct7pX6W2IB+QzN/Ppvi4TV5AxDgLznMZnVQd2PW/4WpUSx/ CXauGusV3n2M/Tak1Jp6EmEhOXCgCf6U4I6D6Cj+7hhh1j77nMXIA0bUx28u/bRol6zXZdTJ lIZ/gIqrLMu7wq7Q9/lRRq6rXWY+BkGVLJt//YS7QiMzu/R/FyfD21dFDpZMoR67IkxWCAg0 UKPk5XxHztzvbaJSHWbsLCJsTe1PitTJmgHDcMZcTY4DxDYiNlbpnryohxLTMZZUvWd9enM/ g23
  • Ironport-hdrordr: A9a23:t3g80K2k6kr/wmj5T7cNSgqjBEgkLtp133Aq2lEZdPU0SKGlfg 6V/MjztCWE7Ar5PUtLpTnuAsa9qB/nm6KdgrNhWItKPjOW21dARbsKheffKlXbcBEWndQtt5 uIHZIeNDXxZ2IK8PoT4mODYqodKA/sytHWuQ/cpU0dMz2Dc8tbnmBE4p7wKDwMeOFBb6BJcq a01458iBeLX28YVci/DmltZZm4mzWa/KiWGCLvHnQcmXGzsQ8=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote:
> On 16.05.2022 16:31, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/flushtlb.h
> > +++ b/xen/arch/x86/include/asm/flushtlb.h
> > @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, 
> > unsigned int flags);
> >  #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags)
> >  
> >  /* Flush all CPUs' TLBs/caches */
> > -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, 
> > flags)
> > +#define flush_area(va, flags) \
> > +    flush_area_mask(&cpu_online_map, (const void *)(va), flags)
> 
> I have to admit that I would prefer if we kept the "_all" name suffix,
> to continue to clearly express the scope of the flush. I'm also not
> really happy to see the cast being added globally now.

But there where no direct callers of flush_area_all(), so the name was
just relevant for it's use in flush_area().  With that now gone I
don't see a need for a flush_area_all(), as flush_area_mask() is more
appropriate.

> > --- a/xen/arch/x86/smp.c
> > +++ b/xen/arch/x86/smp.c
> > @@ -262,7 +262,8 @@ void flush_area_mask(const cpumask_t *mask, const void 
> > *va, unsigned int flags)
> >  {
> >      unsigned int cpu = smp_processor_id();
> >  
> > -    ASSERT(local_irq_is_enabled());
> > +    /* Local flushes can be performed with interrupts disabled. */
> > +    ASSERT(local_irq_is_enabled() || cpumask_equal(mask, cpumask_of(cpu)));
> 
> Further down we use cpumask_subset(mask, cpumask_of(cpu)),
> apparently to also cover the case where mask is empty. I think
> you want to do so here as well.

Hm, yes.  I guess that's cheaper than adding an extra:

if ( cpumask_empty() )
    return;

check at the start of the function.

> >      if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) &&
> >           cpumask_test_cpu(cpu, mask) )
> 
> I suppose we want a further precaution here: Despite the
> !cpumask_subset(mask, cpumask_of(cpu)) below I think we want to
> extend what c64bf2d2a625 ("x86: make CPU state flush requests
> explicit") and later changes (isolating uses of FLUSH_VCPU_STATE
> from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE
> for the local CPU altogether.

If we really want to exclude the use of FLUSH_VCPU_STATE for the local
CPU, we might wish to add this as a separate ASSERT, so that such
checking doesn't depend on !local_irq_is_enabled():

ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu));
ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & FLUSH_VCPU_STATE));


> That's because if such somehow made
> it into the conditional below here, it would still involve an IPI.

Sorry, I'm confused by this: if the mask is empty there should be no
IPI involved at all?  And we shouldn't even get into the second
conditional on the function.

Thanks, Roger.



 


Rackspace

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