From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH 11/15] mm, dax, pmem: introduce __pfn_t References: <20150923043737.36490.70547.stgit@dwillia2-desk3.jf.intel.com> <20150923044211.36490.18084.stgit@dwillia2-desk3.jf.intel.com> From: Dave Hansen Message-ID: <5602CD09.3080801@sr71.net> Date: Wed, 23 Sep 2015 09:02:17 -0700 MIME-Version: 1.0 In-Reply-To: <20150923044211.36490.18084.stgit@dwillia2-desk3.jf.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams , akpm@linux-foundation.org Cc: linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Ross Zwisler , Christoph Hellwig List-ID: On 09/22/2015 09:42 PM, Dan Williams wrote: > /* > + * __pfn_t: encapsulates a page-frame number that is optionally backed > + * by memmap (struct page). Whether a __pfn_t has a 'struct page' > + * backing is indicated by flags in the low bits of the value; > + */ > +typedef struct { > + unsigned long val; > +} __pfn_t; > + > +/* > + * PFN_SG_CHAIN - pfn is a pointer to the next scatterlist entry > + * PFN_SG_LAST - pfn references a page and is the last scatterlist entry > + * PFN_DEV - pfn is not covered by system memmap by default > + * PFN_MAP - pfn has a dynamic page mapping established by a device driver > + */ > +enum { > + PFN_SHIFT = 4, > + PFN_MASK = (1UL << PFN_SHIFT) - 1, > + PFN_SG_CHAIN = (1UL << 0), > + PFN_SG_LAST = (1UL << 1), > + PFN_DEV = (1UL << 2), > + PFN_MAP = (1UL << 3), > +}; Please forgive a little bikeshedding here... Why __pfn_t? Because the KVM code has a pfn_t? If so, I think we should rescue pfn_t from KVM and give them a kvm_pfn_t. I think you should do one of two things: Make PFN_SHIFT 12 so that a physical addr can be stored in a __pfn_t with no work. Or, use the *high* 12 bits of __pfn_t.val. If you use the high bits, *and* make it store a plain pfn when all the bits are 0, then you get a zero-cost pfn<->__pfn_t conversion which will hopefully generate the exact same code which is there today. The one disadvantage here is that it makes it more likely that somebody that's just setting __pfn_t.val=foo will get things subtly wrong somehow, but that it will work most of the time. Also, about naming... PFN_SHIFT is pretty awful name for this. It probably needs to be __PFN_T_SOMETHING. We don't want folks doing craziness like: unsigned long phys_addr = pfn << PFN_SHIFT. Which *looks* OK. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755509AbbIWQCW (ORCPT ); Wed, 23 Sep 2015 12:02:22 -0400 Received: from www.sr71.net ([198.145.64.142]:49372 "EHLO blackbird.sr71.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753940AbbIWQCV (ORCPT ); Wed, 23 Sep 2015 12:02:21 -0400 Subject: Re: [PATCH 11/15] mm, dax, pmem: introduce __pfn_t To: Dan Williams , akpm@linux-foundation.org References: <20150923043737.36490.70547.stgit@dwillia2-desk3.jf.intel.com> <20150923044211.36490.18084.stgit@dwillia2-desk3.jf.intel.com> Cc: linux-nvdimm@ml01.01.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Ross Zwisler , Christoph Hellwig From: Dave Hansen Message-ID: <5602CD09.3080801@sr71.net> Date: Wed, 23 Sep 2015 09:02:17 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150923044211.36490.18084.stgit@dwillia2-desk3.jf.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/22/2015 09:42 PM, Dan Williams wrote: > /* > + * __pfn_t: encapsulates a page-frame number that is optionally backed > + * by memmap (struct page). Whether a __pfn_t has a 'struct page' > + * backing is indicated by flags in the low bits of the value; > + */ > +typedef struct { > + unsigned long val; > +} __pfn_t; > + > +/* > + * PFN_SG_CHAIN - pfn is a pointer to the next scatterlist entry > + * PFN_SG_LAST - pfn references a page and is the last scatterlist entry > + * PFN_DEV - pfn is not covered by system memmap by default > + * PFN_MAP - pfn has a dynamic page mapping established by a device driver > + */ > +enum { > + PFN_SHIFT = 4, > + PFN_MASK = (1UL << PFN_SHIFT) - 1, > + PFN_SG_CHAIN = (1UL << 0), > + PFN_SG_LAST = (1UL << 1), > + PFN_DEV = (1UL << 2), > + PFN_MAP = (1UL << 3), > +}; Please forgive a little bikeshedding here... Why __pfn_t? Because the KVM code has a pfn_t? If so, I think we should rescue pfn_t from KVM and give them a kvm_pfn_t. I think you should do one of two things: Make PFN_SHIFT 12 so that a physical addr can be stored in a __pfn_t with no work. Or, use the *high* 12 bits of __pfn_t.val. If you use the high bits, *and* make it store a plain pfn when all the bits are 0, then you get a zero-cost pfn<->__pfn_t conversion which will hopefully generate the exact same code which is there today. The one disadvantage here is that it makes it more likely that somebody that's just setting __pfn_t.val=foo will get things subtly wrong somehow, but that it will work most of the time. Also, about naming... PFN_SHIFT is pretty awful name for this. It probably needs to be __PFN_T_SOMETHING. We don't want folks doing craziness like: unsigned long phys_addr = pfn << PFN_SHIFT. Which *looks* OK.