From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751701AbcFNVhv (ORCPT ); Tue, 14 Jun 2016 17:37:51 -0400 Received: from mga11.intel.com ([192.55.52.93]:29965 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750944AbcFNVhu (ORCPT ); Tue, 14 Jun 2016 17:37:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,472,1459839600"; d="scan'208";a="975715717" Subject: Re: [PATCH] Linux VM workaround for Knights Landing A/D leak To: Nadav Amit References: <1465919919-2093-1-git-send-email-lukasz.anaczkowski@intel.com> <7FB15233-B347-4A87-9506-A9E10D331292@gmail.com> <57603C61.5000408@linux.intel.com> <2471A3E8-FF69-4720-A3BF-BDC6094A6A70@gmail.com> Cc: Lukasz Anaczkowski , LKML , linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , ak@linux.intel.com, kirill.shutemov@linux.intel.com, mhocko@suse.com, Andrew Morton , "H. Peter Anvin" , harish.srinivasappa@intel.com, lukasz.odzioba@intel.com, Andy Lutomirski From: Dave Hansen Message-ID: <5760792D.90000@linux.intel.com> Date: Tue, 14 Jun 2016 14:37:49 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <2471A3E8-FF69-4720-A3BF-BDC6094A6A70@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/14/2016 01:16 PM, Nadav Amit wrote: > Dave Hansen wrote: > >> On 06/14/2016 09:47 AM, Nadav Amit wrote: >>> Lukasz Anaczkowski wrote: >>> >>>>> From: Andi Kleen >>>>> +void fix_pte_leak(struct mm_struct *mm, unsigned long addr, pte_t *ptep) >>>>> +{ >>> Here there should be a call to smp_mb__after_atomic() to synchronize with >>> switch_mm. I submitted a similar patch, which is still pending (hint). >>> >>>>> + if (cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids) { >>>>> + trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL); >>>>> + flush_tlb_others(mm_cpumask(mm), mm, addr, >>>>> + addr + PAGE_SIZE); >>>>> + mb(); >>>>> + set_pte(ptep, __pte(0)); >>>>> + } >>>>> +} >> >> Shouldn't that barrier be incorporated in the TLB flush code itself and >> not every single caller (like this code is)? >> >> It is insane to require individual TLB flushers to be concerned with the >> barriers. > > IMHO it is best to use existing flushing interfaces instead of creating > new ones. Yeah, or make these things a _little_ harder to get wrong. That little snippet above isn't so crazy that we should be depending on open-coded barriers to get it right. Should we just add a barrier to mm_cpumask() itself? That should stop the race. Or maybe we need a new primitive like: /* * Call this if a full barrier has been executed since the last * pagetable modification operation. */ static int __other_cpus_need_tlb_flush(struct mm_struct *mm) { /* cpumask_any_but() returns >= nr_cpu_ids if no cpus set. */ return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids; } static int other_cpus_need_tlb_flush(struct mm_struct *mm) { /* * Synchronizes with switch_mm. Makes sure that we do not * observe a bit having been cleared in mm_cpumask() before * the other processor has seen our pagetable update. See * switch_mm(). */ smp_mb__after_atomic(); return __other_cpus_need_tlb_flush(mm) } We should be able to deploy other_cpus_need_tlb_flush() in most of the cases where we are doing "cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids". Right? > In theory, fix_pte_leak could have used flush_tlb_page. But the problem > is that flush_tlb_page requires the vm_area_struct as an argument, which > ptep_get_and_clear (and others) do not have. That, and we do not want/need to flush the _current_ processor's TLB. flush_tlb_page() would have done that unnecessarily. That's not the end of the world here, but it is a downside.