Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755164AbdCQBAf convert rfc822-to-8bit (ORCPT ); Thu, 16 Mar 2017 21:00:35 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:17340 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753311AbdCQBAd (ORCPT ); Thu, 16 Mar 2017 21:00:33 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Thu, 16 Mar 2017 18:00:06 -0700 Subject: Re: [HMM 07/16] mm/migrate: new memory migration helper for use with device memory v4 To: Balbir Singh References: <1489680335-6594-1-git-send-email-jglisse@redhat.com> <1489680335-6594-8-git-send-email-jglisse@redhat.com> <20170316160520.d03ac02474cad6d2c8eba9bc@linux-foundation.org> CC: Andrew Morton , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , "linux-kernel@vger.kernel.org" , linux-mm , Naoya Horiguchi , David Nellans , Evgeny Baskakov , Mark Hairgrove , Sherry Cheung , Subhash Gutti From: John Hubbard X-Nvconfidentiality: public Message-ID: <94e0d115-7deb-c748-3dc2-60d6289e6551@nvidia.com> Date: Thu, 16 Mar 2017 17:57:35 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [172.17.160.221] X-ClientProxiedBy: HQMAIL101.nvidia.com (172.20.187.10) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3295 Lines: 104 On 03/16/2017 05:45 PM, Balbir Singh wrote: > On Fri, Mar 17, 2017 at 11:22 AM, John Hubbard wrote: >> On 03/16/2017 04:05 PM, Andrew Morton wrote: >>> >>> On Thu, 16 Mar 2017 12:05:26 -0400 Jérôme Glisse >>> wrote: >>> >>>> +static inline struct page *migrate_pfn_to_page(unsigned long mpfn) >>>> +{ >>>> + if (!(mpfn & MIGRATE_PFN_VALID)) >>>> + return NULL; >>>> + return pfn_to_page(mpfn & MIGRATE_PFN_MASK); >>>> +} >>> >>> >>> i386 allnoconfig: >>> >>> In file included from mm/page_alloc.c:61: >>> ./include/linux/migrate.h: In function 'migrate_pfn_to_page': >>> ./include/linux/migrate.h:139: warning: left shift count >= width of type >>> ./include/linux/migrate.h:141: warning: left shift count >= width of type >>> ./include/linux/migrate.h: In function 'migrate_pfn_size': >>> ./include/linux/migrate.h:146: warning: left shift count >= width of type >>> >> >> It seems clear that this was never meant to work with < 64-bit pfns: >> >> // migrate.h excerpt: >> #define MIGRATE_PFN_VALID (1UL << (BITS_PER_LONG_LONG - 1)) >> #define MIGRATE_PFN_MIGRATE (1UL << (BITS_PER_LONG_LONG - 2)) >> #define MIGRATE_PFN_HUGE (1UL << (BITS_PER_LONG_LONG - 3)) >> #define MIGRATE_PFN_LOCKED (1UL << (BITS_PER_LONG_LONG - 4)) >> #define MIGRATE_PFN_WRITE (1UL << (BITS_PER_LONG_LONG - 5)) >> #define MIGRATE_PFN_DEVICE (1UL << (BITS_PER_LONG_LONG - 6)) >> #define MIGRATE_PFN_ERROR (1UL << (BITS_PER_LONG_LONG - 7)) >> #define MIGRATE_PFN_MASK ((1UL << (BITS_PER_LONG_LONG - PAGE_SHIFT)) >> - 1) >> >> ...obviously, there is not enough room for these flags, in a 32-bit pfn. >> >> So, given the current HMM design, I think we are going to have to provide a >> 32-bit version of these routines (migrate_pfn_to_page, and related) that is >> a no-op, right? > > Or make the HMM Kconfig feature 64BIT only by making it depend on 64BIT? > Yes, that was my first reaction too, but these particular routines are aspiring to be generic routines--in fact, you have had an influence there, because these might possibly help with NUMA migrations. :) So it would look odd to see this: #ifdef CONFIG_HMM int migrate_vma(const struct migrate_vma_ops *ops, struct vm_area_struct *vma, unsigned long mentries, unsigned long start, unsigned long end, unsigned long *src, unsigned long *dst, void *private) { //...implementation #endif ...because migrate_vma() does not sound HMM-specific, and it is, after all, in migrate.h and migrate.c. We probably want this a more generic approach (not sure if I've picked exactly the right token to #ifdef on, but it's close): #ifdef CONFIG_64BIT int migrate_vma(const struct migrate_vma_ops *ops, struct vm_area_struct *vma, unsigned long mentries, unsigned long start, unsigned long end, unsigned long *src, unsigned long *dst, void *private) { /* ... full implementation */ } #else int migrate_vma(const struct migrate_vma_ops *ops, struct vm_area_struct *vma, unsigned long mentries, unsigned long start, unsigned long end, unsigned long *src, unsigned long *dst, void *private) { return -EINVAL; /* or something more appropriate */ } #endif thanks John Hubbard NVIDIA > > Balbir Singh >