Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753394AbdDJVfk (ORCPT ); Mon, 10 Apr 2017 17:35:40 -0400 Received: from mail-oi0-f45.google.com ([209.85.218.45]:36327 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752940AbdDJVfi (ORCPT ); Mon, 10 Apr 2017 17:35:38 -0400 MIME-Version: 1.0 In-Reply-To: References: <149161025237.38725.13508986873214668503.stgit@dwillia2-desk3.amr.corp.intel.com> From: Dan Williams Date: Mon, 10 Apr 2017 14:35:36 -0700 Message-ID: Subject: Re: [PATCH v2] x86, pmem: fix broken __copy_user_nocache cache-bypass assumptions To: "Kani, Toshimitsu" Cc: "linux-nvdimm@lists.01.org" , Jan Kara , Matthew Wilcox , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Christoph Hellwig , Jeff Moyer , Ingo Molnar , Al Viro , "H. Peter Anvin" , Thomas Gleixner , Ross Zwisler 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: 1666 Lines: 45 On Mon, Apr 10, 2017 at 2:28 PM, Kani, Toshimitsu wrote: >> > Thanks for the update. I think the alignment check should be based on >> > the following note in copy_user_nocache. >> > >> > * Note: Cached memory copy is used when destination or size is not >> > * naturally aligned. That is: >> > * - Require 8-byte alignment when size is 8 bytes or larger. >> > * - Require 4-byte alignment when size is 4 bytes. >> > >> > So, I think the code may be something like this. I also made the following >> changes: >> >> Thanks! >> >> > - Mask with 7, not 8. >> >> Yes, good catch. >> >> > - ALIGN with cacheline size, instead of 8. >> > - Add (bytes > flushed) test since calculation with unsigned long still results >> in a negative >> > value (as a positive value). >> > >> > if (bytes < 8) { >> > if ((dest & 3) || (bytes != 4)) >> > arch_wb_cache_pmem(addr, 1); >> > } else { >> > if (dest & 7) { >> > dest = ALIGN(dest, boot_cpu_data.x86_clflush_size); >> >> Why align the destination to the next cacheline? As far as I can see >> the ALIGN_DESTINATION macro in arch/x86/include/asm/asm.h only aligns >> to the next 8-byte boundary. > > The clflush here flushes for the cacheline size. So, we do not need to flush > the same cacheline again when the unaligned tail is in the same line. Ok, makes sense. Last question, can't we reduce the check to be: if ((bytes > flushed) && ((bytes - flushed) & 3)) ...since if 'bytes' was 4-byte aligned we would have performed non-temporal stores. Can I add your Signed-off-by: on v3?