From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4D16C433DF for ; Fri, 26 Jun 2020 14:17:26 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 87D3C208C9 for ; Fri, 26 Jun 2020 14:17:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 87D3C208C9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jopAB-0007GE-EJ; Fri, 26 Jun 2020 14:17:07 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jopAA-0007G9-6F for xen-devel@lists.xenproject.org; Fri, 26 Jun 2020 14:17:06 +0000 X-Inumbo-ID: b5e1c43e-b7b7-11ea-8496-bc764e2007e4 Received: from esa1.hc3370-68.iphmx.com (unknown [216.71.145.142]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id b5e1c43e-b7b7-11ea-8496-bc764e2007e4; Fri, 26 Jun 2020 14:17:05 +0000 (UTC) Authentication-Results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: U+C7AUTwewX4QFv6J22n3BMoAsv62KbOnDYUSRmS/h8iZ3xEAK1wUPj+fY/a6hCI8RbebRHvEX NnbiEbjxc+cHvaDoy4SsM5tlDLjDYywpnkKc5vHF1nlOvnvaCxOIRAhiVGf7H/LjSmQ7GlTosN RKP3jaeC3F8J/tmscjl+VDyFpXxloeiQpPL06MRxUTmKIV6uiXgvKrrQEODqOmwgIBgV7fWSFZ cc50W9FuISa9nb2D9/YAg93QmCgmFnt/Eqq+4bjafW8kXYA/GrzA7KF/voL4HDYlrdTzxef3cB SXo= X-SBRS: 2.7 X-MesageID: 21334863 X-Ironport-Server: esa1.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.75,283,1589256000"; d="scan'208";a="21334863" Date: Fri, 26 Jun 2020 16:16:56 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Julien Grall Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage Message-ID: <20200626141656.GC735@Air-de-Roger> References: <20200625113041.81507-1-roger.pau@citrix.com> <551387c6-f45d-bf6c-a41e-b0920425db9f@xen.org> <20200626100745.GB735@Air-de-Roger> <5586cae5-8929-0c53-7a35-5dd6116c77c2@suse.com> <000b01d64bbc$a7822f30$f6868d90$@xen.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To AMSPEX02CL02.citrite.net (10.69.22.126) X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: 'Stefano Stabellini' , 'Wei Liu' , paul@xen.org, 'Andrew Cooper' , 'Ian Jackson' , 'George Dunlap' , 'Jan Beulich' , xen-devel@lists.xenproject.org, 'Volodymyr Babchuk' Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Fri, Jun 26, 2020 at 02:58:21PM +0100, Julien Grall wrote: > On 26/06/2020 14:21, Paul Durrant wrote: > > > -----Original Message----- > > > From: Jan Beulich > > > Sent: 26 June 2020 14:11 > > > To: Roger Pau Monné ; paul@xen.org; Andrew Cooper > > > Cc: Julien Grall ; xen-devel@lists.xenproject.org; Wei Liu ; George Dunlap > > > ; Ian Jackson ; Stefano Stabellini > > > ; Volodymyr Babchuk > > > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage > > > > > > On 26.06.2020 12:07, Roger Pau Monné wrote: > > > > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: > > > > > Hi Roger, > > > > > > > > > > Sorry I didn't manage to answer to your question before you sent v3. > > > > > > > > > > On 25/06/2020 12:30, Roger Pau Monne wrote: > > > > > > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > > > > > > index ab1aae5c90..7ae0543885 100644 > > > > > > --- a/xen/include/asm-arm/flushtlb.h > > > > > > +++ b/xen/include/asm-arm/flushtlb.h > > > > > > @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > > > > > > /* Flush specified CPUs' TLBs */ > > > > > > void flush_tlb_mask(const cpumask_t *mask); > > > > > > +#define flush_tlb_mask_sync flush_tlb_mask > > > > > > > > > > Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice > > > > > improvement, but it unfortunately doesn't fully address my concern. > > > > > > > > > > After this patch there is exactly one use of flush_tlb_mask() in common code > > > > > (see grant_table.c). But without looking at the x86 code, it is not clear > > > > > why this requires a different flush compare to the two other sites. > > > > > > > > It's not dealing with page allocation or page type changes directly, > > > > and hence doesn't need to use an IPI in order to prevent races with > > > > spurious_page_fault. > > > > > > > > > IOW, if I want to modify the common code in the future, how do I know which > > > > > flush to call? > > > > > > > > Unless you modify one of the specific areas mentioned above (page > > > > allocation or page type changes) you should use flush_tlb_mask. > > > > > > > > This is not ideal, and my aim will be to be able to use the assisted > > > > flush everywhere if possible, so I would really like to get rid of the > > > > interrupt disabling done in spurious_page_fault and this model where > > > > x86 relies on blocking interrupts in order to prevent page type > > > > changes or page freeing. > > > > > > > > Such change however doesn't feel appropriate for a release freeze > > > > period, and hence went with something smaller that restores the > > > > previous behavior. Another option is to just disable assisted flushes > > > > for the time being and re-enable them when a suitable solution is > > > > found. > > > > > > As I can understand Julien's concern, maybe this would indeed be > > > the better approach for now? Andrew, Paul - thoughts? > > > > > > > Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 (which I think will address Julien's concern)? > Bear in mind that while this may be properly fixed in the next release, the > hack will stay forever in Xen 4.14. > > While I understand that disabling assisted flush is going to have a > performance impact, we also need to make sure the interface make senses. > > From a generic perspective, a TLB flush should have the exact same guarantee > regardless where we call it in common/. So I would still strongly prefer if > we have a single helper. > > Is it possible to consider to replace all the flush_tlb_mask() in common > code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86, > this would be an alias to flush_tlb_mask_sync()? The TLB flush call in grant_table.c could still use a flush_tlb_mask, but it will also work fine with a flush_tlb_mask_sync. I can prepare a patch if that's acceptable, I guess it would be slightly better than fully disabling assisted flush. Roger.