All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joao Martins <joao.m.martins@oracle.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-nvdimm@lists.01.org,
	Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	kvm@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Liran Alon <liran.alon@oracle.com>,
	Nikita Leshenko <nikita.leshchenko@oracle.com>,
	Barret Rhoden <brho@google.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH RFC 01/10] mm: Add pmd support for _PAGE_SPECIAL
Date: Tue, 4 Feb 2020 16:14:18 +0000	[thread overview]
Message-ID: <82a41394-76c3-d643-3b49-af37a2036b73@oracle.com> (raw)
In-Reply-To: <20200203213442.GK8731@bombadil.infradead.org>

On 2/3/20 9:34 PM, Matthew Wilcox wrote:
> On Fri, Jan 10, 2020 at 07:03:04PM +0000, Joao Martins wrote:
>> +++ b/arch/x86/include/asm/pgtable.h
>> @@ -293,6 +293,15 @@ static inline int pgd_devmap(pgd_t pgd)
>>  {
>>  	return 0;
>>  }
>> +#endif
>> +
>> +#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
>> +static inline int pmd_special(pmd_t pmd)
>> +{
>> +	return !!(pmd_flags(pmd) & _PAGE_SPECIAL);
>> +}
>> +#endif
> 
> The ifdef/endif don't make much sense here; x86 does have PTE_SPECIAL,
> and this is an x86 header file, so that can be assumed.
> 
Gotcha.

>> +++ b/mm/gup.c
>> @@ -2079,6 +2079,9 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
>>  		return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
>>  	}
>>  
>> +	if (pmd_special(orig))
>> +		return 0;
> 
> Here, you're calling it unconditionally.  I think you need a pmd_special()
> conditionally defined in include/asm-generic/pgtable.h
> 
> +#ifndef CONFIG_ARCH_HAS_PTE_SPECIAL
> +static inline bool pmd_special(pmd_t pmd)
> +{
> +	return false;
> +}
> +#endif
> 
> (oh, and plese use bool instead of int; I know that's different from
> pte_special(), but pte_special() predates bool and nobody's done the work
> to convert it yet)
> 
Got it.

>> +++ b/mm/huge_memory.c
>> @@ -791,6 +791,8 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
>>  	entry = pmd_mkhuge(pfn_t_pmd(pfn, prot));
>>  	if (pfn_t_devmap(pfn))
>>  		entry = pmd_mkdevmap(entry);
>> +	else if (pfn_t_special(pfn))
>> +		entry = pmd_mkspecial(entry);
> 
> Again, we'll need a generic one.
> 
Will add it.

>> @@ -823,8 +825,7 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
>>  	 * but we need to be consistent with PTEs and architectures that
>>  	 * can't support a 'special' bit.
>>  	 */
>> -	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) &&
>> -			!pfn_t_devmap(pfn));
>> +	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
> 
> Should that rather be ...
> 
> +	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) &&
> +			!pfn_t_devmap(pfn) && !pfn_t_special(pfn));
> 
Yes. That is indeed a mistake I had already fixed for v2. Patch 3 does the exact
same, so as the other comments you mentioned here too so will adjust that
accordingly.

> I also think this comment needs adjusting:
> 
>         /*
>          * There is no pmd_special() but there may be special pmds, e.g.
>          * in a direct-access (dax) mapping, so let's just replicate the
>          * !CONFIG_ARCH_HAS_PTE_SPECIAL case from vm_normal_page() here.
>          */
> 
> 
I'll replace with what vm_normal_page() equivalent has:

	/* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */

  Joao
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

WARNING: multiple messages have this Message-ID (diff)
From: Joao Martins <joao.m.martins@oracle.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-nvdimm@lists.01.org,
	Dan Williams <dan.j.williams@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	kvm@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Liran Alon <liran.alon@oracle.com>,
	Nikita Leshenko <nikita.leshchenko@oracle.com>,
	Barret Rhoden <brho@google.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH RFC 01/10] mm: Add pmd support for _PAGE_SPECIAL
Date: Tue, 4 Feb 2020 16:14:18 +0000	[thread overview]
Message-ID: <82a41394-76c3-d643-3b49-af37a2036b73@oracle.com> (raw)
In-Reply-To: <20200203213442.GK8731@bombadil.infradead.org>

On 2/3/20 9:34 PM, Matthew Wilcox wrote:
> On Fri, Jan 10, 2020 at 07:03:04PM +0000, Joao Martins wrote:
>> +++ b/arch/x86/include/asm/pgtable.h
>> @@ -293,6 +293,15 @@ static inline int pgd_devmap(pgd_t pgd)
>>  {
>>  	return 0;
>>  }
>> +#endif
>> +
>> +#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
>> +static inline int pmd_special(pmd_t pmd)
>> +{
>> +	return !!(pmd_flags(pmd) & _PAGE_SPECIAL);
>> +}
>> +#endif
> 
> The ifdef/endif don't make much sense here; x86 does have PTE_SPECIAL,
> and this is an x86 header file, so that can be assumed.
> 
Gotcha.

>> +++ b/mm/gup.c
>> @@ -2079,6 +2079,9 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
>>  		return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
>>  	}
>>  
>> +	if (pmd_special(orig))
>> +		return 0;
> 
> Here, you're calling it unconditionally.  I think you need a pmd_special()
> conditionally defined in include/asm-generic/pgtable.h
> 
> +#ifndef CONFIG_ARCH_HAS_PTE_SPECIAL
> +static inline bool pmd_special(pmd_t pmd)
> +{
> +	return false;
> +}
> +#endif
> 
> (oh, and plese use bool instead of int; I know that's different from
> pte_special(), but pte_special() predates bool and nobody's done the work
> to convert it yet)
> 
Got it.

>> +++ b/mm/huge_memory.c
>> @@ -791,6 +791,8 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
>>  	entry = pmd_mkhuge(pfn_t_pmd(pfn, prot));
>>  	if (pfn_t_devmap(pfn))
>>  		entry = pmd_mkdevmap(entry);
>> +	else if (pfn_t_special(pfn))
>> +		entry = pmd_mkspecial(entry);
> 
> Again, we'll need a generic one.
> 
Will add it.

>> @@ -823,8 +825,7 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
>>  	 * but we need to be consistent with PTEs and architectures that
>>  	 * can't support a 'special' bit.
>>  	 */
>> -	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) &&
>> -			!pfn_t_devmap(pfn));
>> +	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
> 
> Should that rather be ...
> 
> +	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) &&
> +			!pfn_t_devmap(pfn) && !pfn_t_special(pfn));
> 
Yes. That is indeed a mistake I had already fixed for v2. Patch 3 does the exact
same, so as the other comments you mentioned here too so will adjust that
accordingly.

> I also think this comment needs adjusting:
> 
>         /*
>          * There is no pmd_special() but there may be special pmds, e.g.
>          * in a direct-access (dax) mapping, so let's just replicate the
>          * !CONFIG_ARCH_HAS_PTE_SPECIAL case from vm_normal_page() here.
>          */
> 
> 
I'll replace with what vm_normal_page() equivalent has:

	/* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */

  Joao

  reply	other threads:[~2020-02-04 16:15 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 19:03 [PATCH RFC 00/10] device-dax: Support devices without PFN metadata Joao Martins
2020-01-10 19:03 ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 01/10] mm: Add pmd support for _PAGE_SPECIAL Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-02-03 21:34   ` Matthew Wilcox
2020-02-03 21:34     ` Matthew Wilcox
2020-02-04 16:14     ` Joao Martins [this message]
2020-02-04 16:14       ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 02/10] mm: Handle pmd entries in follow_pfn() Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-02-03 21:37   ` Matthew Wilcox
2020-02-03 21:37     ` Matthew Wilcox
2020-02-04 16:17     ` Joao Martins
2020-02-04 16:17       ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 03/10] mm: Add pud support for _PAGE_SPECIAL Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 04/10] mm: Handle pud entries in follow_pfn() Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 05/10] device-dax: Do not enforce MADV_DONTFORK on mmap() Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 06/10] device-dax: Introduce pfn_flags helper Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 07/10] device-dax: Add support for PFN_SPECIAL flags Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 08/10] dax/pmem: Add device-dax support for PFN_MODE_NONE Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-01-10 19:03 ` [PATCH RFC 09/10] vfio/type1: Use follow_pfn for VM_FPNMAP VMAs Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-02-07 21:08   ` Jason Gunthorpe
2020-02-11 16:23     ` Joao Martins
2020-02-11 16:23       ` Joao Martins
2020-02-11 16:50       ` Jason Gunthorpe
2020-01-10 19:03 ` [PATCH RFC 10/10] nvdimm/e820: add multiple namespaces support Joao Martins
2020-01-10 19:03   ` Joao Martins
2020-02-04 15:28   ` Barret Rhoden
2020-02-04 15:28     ` Barret Rhoden
2020-02-04 16:44     ` Dan Williams
2020-02-04 16:44       ` Dan Williams
2020-02-04 16:44       ` Dan Williams
2020-02-04 18:20       ` Barret Rhoden
2020-02-04 18:20         ` Barret Rhoden
2020-02-04 19:24         ` Joao Martins
2020-02-04 19:24           ` Joao Martins
2020-02-04 21:43         ` Dan Williams
2020-02-04 21:43           ` Dan Williams
2020-02-04 21:43           ` Dan Williams
2020-02-04 21:57           ` Barret Rhoden
2020-02-04 21:57             ` Barret Rhoden
2020-02-04  1:24 ` [PATCH RFC 00/10] device-dax: Support devices without PFN metadata Dan Williams
2020-02-04  1:24   ` Dan Williams
2020-02-04  1:24   ` Dan Williams
2020-02-04 19:07   ` Joao Martins
2020-02-04 19:07     ` Joao Martins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=82a41394-76c3-d643-3b49-af37a2036b73@oracle.com \
    --to=joao.m.martins@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex.williamson@redhat.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=bp@alien8.de \
    --cc=brho@google.com \
    --cc=cohuck@redhat.com \
    --cc=hpa@zytor.com \
    --cc=konrad.wilk@oracle.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=liran.alon@oracle.com \
    --cc=mingo@redhat.com \
    --cc=nikita.leshchenko@oracle.com \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.