Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752751AbdFUPLi (ORCPT ); Wed, 21 Jun 2017 11:11:38 -0400 Received: from mail.kernel.org ([198.145.29.99]:46706 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751133AbdFUPLh (ORCPT ); Wed, 21 Jun 2017 11:11:37 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0277620C4F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=luto@kernel.org MIME-Version: 1.0 In-Reply-To: References: <91f24a6145b2077f992902891f8fa59abe5c8696.1498022414.git.luto@kernel.org> From: Andy Lutomirski Date: Wed, 21 Jun 2017 08:11:15 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 05/11] x86/mm: Track the TLB's tlb_gen and update the flushing algorithm To: Thomas Gleixner Cc: Andy Lutomirski , X86 ML , "linux-kernel@vger.kernel.org" , Borislav Petkov , Linus Torvalds , Andrew Morton , Mel Gorman , "linux-mm@kvack.org" , Nadav Amit , Rik van Riel , Dave Hansen , Arjan van de Ven , Peter Zijlstra Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3878 Lines: 104 On Wed, Jun 21, 2017 at 1:32 AM, Thomas Gleixner wrote: > On Tue, 20 Jun 2017, Andy Lutomirski wrote: >> struct flush_tlb_info { >> + /* >> + * We support several kinds of flushes. >> + * >> + * - Fully flush a single mm. flush_mm will be set, flush_end will be > > flush_mm is the *mm member in the struct, right? You might rename that as a > preparatory step so comments and implementation match. The comment is outdated. Fixed now. > >> + * TLB_FLUSH_ALL, and new_tlb_gen will be the tlb_gen to which the >> + * IPI sender is trying to catch us up. >> + * >> + * - Partially flush a single mm. flush_mm will be set, flush_start >> + * and flush_end will indicate the range, and new_tlb_gen will be >> + * set such that the changes between generation new_tlb_gen-1 and >> + * new_tlb_gen are entirely contained in the indicated range. >> + * >> + * - Fully flush all mms whose tlb_gens have been updated. flush_mm >> + * will be NULL, flush_end will be TLB_FLUSH_ALL, and new_tlb_gen >> + * will be zero. >> + */ >> struct mm_struct *mm; >> unsigned long start; >> unsigned long end; >> + u64 new_tlb_gen; > > Nit. While at it could you please make that struct tabular aligned as we > usually do in x86? Sure. > >> static void flush_tlb_func_common(const struct flush_tlb_info *f, >> bool local, enum tlb_flush_reason reason) >> { >> + struct mm_struct *loaded_mm = this_cpu_read(cpu_tlbstate.loaded_mm); >> + >> + /* >> + * Our memory ordering requirement is that any TLB fills that >> + * happen after we flush the TLB are ordered after we read >> + * active_mm's tlb_gen. We don't need any explicit barrier >> + * because all x86 flush operations are serializing and the >> + * atomic64_read operation won't be reordered by the compiler. >> + */ > > Can you please move the comment above the loaded_mm assignment? I'll move it above the function entirely. It's more of a general comment about how the function works than any particular part of the function. > >> + u64 mm_tlb_gen = atomic64_read(&loaded_mm->context.tlb_gen); >> + u64 local_tlb_gen = this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen); >> + >> /* This code cannot presently handle being reentered. */ >> VM_WARN_ON(!irqs_disabled()); >> >> + VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) != >> + loaded_mm->context.ctx_id); >> + >> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) { >> + /* >> + * leave_mm() is adequate to handle any type of flush, and >> + * we would prefer not to receive further IPIs. > > While I know what you mean, it might be useful to have a more elaborate > explanation why this prevents new IPIs. Added, although it just gets deleted again later in the series. > >> + */ >> leave_mm(smp_processor_id()); >> return; >> } >> >> - if (f->end == TLB_FLUSH_ALL) { >> - local_flush_tlb(); >> - if (local) >> - count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL); >> - trace_tlb_flush(reason, TLB_FLUSH_ALL); >> - } else { >> + if (local_tlb_gen == mm_tlb_gen) { >> + /* >> + * There's nothing to do: we're already up to date. This can >> + * happen if two concurrent flushes happen -- the first IPI to >> + * be handled can catch us all the way up, leaving no work for >> + * the second IPI to be handled. > > That not restricted to IPIs, right? A local flush / IPI combo can do that > as well. Indeed. Comment fixed. > > Other than those nits; > > Reviewed-by: Thomas Gleixner