[PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Huang Ying]

Make __get_user_pages return -EHWPOISON for HWPOISON page only if
FOLL_HWPOISON is specified.  With this patch, the interested callers
can distinguish HWPOISON page from general FAULT page, while other
callers will still get -EFAULT for pages, so the user space interface
need not to be changed.

get_user_pages_hwpoison is added as a variant of get_user_pages that
can return -EHWPOISON for HWPOISON page.

This feature is needed by KVM, where UCR MCE should be relayed to
guest for HWPOISON page, while instruction emulation and MMIO will be
tried for general FAULT page.

The idea comes from Andrew Morton.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/asm-generic/errno.h |    2 +
 include/linux/mm.h          |   17 +++++++++++++
 mm/memory.c                 |   55 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 71 insertions(+), 3 deletions(-)

--- a/include/asm-generic/errno.h
+++ b/include/asm-generic/errno.h
@@ -108,4 +108,6 @@
 
 #define ERFKILL		132	/* Operation not possible due to RF-kill */
 
+#define EHWPOISON	133	/* Memory page has hardware error */
+
 #endif
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -860,6 +860,22 @@ int get_user_pages(struct task_struct *t
 			struct page **pages, struct vm_area_struct **vmas);
 int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			struct page **pages);
+#ifdef CONFIG_MEMORY_FAILURE
+int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
+			    unsigned long start, int nr_pages, int write,
+			    int force, struct page **pages,
+			    struct vm_area_struct **vmas);
+#else
+static inline int get_user_pages_hwpoison(struct task_struct *tsk,
+					  struct mm_struct *mm,
+					  unsigned long start, int nr_pages,
+					  int write, int force,
+					  struct page **pages,
+					  struct vm_area_struct **vmas) {
+	return get_user_pages(tsk, mm, start, nr_pages,
+			      write, force, pages, vmas);
+}
+#endif
 struct page *get_dump_page(unsigned long addr);
 
 extern int try_to_release_page(struct page * page, gfp_t gfp_mask);
@@ -1415,6 +1431,7 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_DUMP	0x08	/* give error on hole if it would be zero */
 #define FOLL_FORCE	0x10	/* get_user_pages read/write w/o permission */
+#define FOLL_HWPOISON	0x20	/* check page is hwpoisoned */
 
 typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
 			void *data);
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1449,9 +1449,16 @@ int __get_user_pages(struct task_struct
 				if (ret & VM_FAULT_ERROR) {
 					if (ret & VM_FAULT_OOM)
 						return i ? i : -ENOMEM;
-					if (ret &
-					    (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE|
-					     VM_FAULT_SIGBUS))
+					if (ret & (VM_FAULT_HWPOISON |
+						   VM_FAULT_HWPOISON_LARGE)) {
+						if (i)
+							return i;
+						else if (gup_flags & FOLL_HWPOISON)
+							return -EHWPOISON;
+						else
+							return -EFAULT;
+					}
+					if (ret & VM_FAULT_SIGBUS)
 						return i ? i : -EFAULT;
 					BUG();
 				}
@@ -1563,6 +1570,48 @@ int get_user_pages(struct task_struct *t
 }
 EXPORT_SYMBOL(get_user_pages);
 
+#ifdef CONFIG_MEMORY_FAILURE
+/**
+ * get_user_pages_hwpoison() - pin user pages in memory, return hwpoison status
+ * @tsk:	task_struct of target task
+ * @mm:		mm_struct of target mm
+ * @start:	starting user address
+ * @nr_pages:	number of pages from start to pin
+ * @write:	whether pages will be written to by the caller
+ * @force:	whether to force write access even if user mapping is
+ *		readonly. This will result in the page being COWed even
+ *		in MAP_SHARED mappings. You do not want this.
+ * @pages:	array that receives pointers to the pages pinned.
+ *		Should be at least nr_pages long. Or NULL, if caller
+ *		only intends to ensure the pages are faulted in.
+ * @vmas:	array of pointers to vmas corresponding to each page.
+ *		Or NULL if the caller does not require them.
+ *
+ * Returns number of pages pinned.
+ *
+ * If the page table or memory page is hwpoisoned, return -EHWPOISON.
+ *
+ * Otherwise, same as get_user_pages.
+ */
+int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
+			    unsigned long start, int nr_pages, int write,
+			    int force, struct page **pages,
+			    struct vm_area_struct **vmas)
+{
+	int flags = FOLL_TOUCH | FOLL_HWPOISON;
+
+	if (pages)
+		flags |= FOLL_GET;
+	if (write)
+		flags |= FOLL_WRITE;
+	if (force)
+		flags |= FOLL_FORCE;
+
+	return __get_user_pages(tsk, mm, start, nr_pages, flags, pages, vmas);
+}
+EXPORT_SYMBOL(get_user_pages_hwpoison);
+#endif
+
 /**
  * get_dump_page() - pin user page in memory while writing it to core dump
  * @addr: user address

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Avi Kivity]

On 01/13/2011 10:42 AM, Huang Ying wrote:
> Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> FOLL_HWPOISON is specified.  With this patch, the interested callers
> can distinguish HWPOISON page from general FAULT page, while other
> callers will still get -EFAULT for pages, so the user space interface
> need not to be changed.
>
> get_user_pages_hwpoison is added as a variant of get_user_pages that
> can return -EHWPOISON for HWPOISON page.
>
> This feature is needed by KVM, where UCR MCE should be relayed to
> guest for HWPOISON page, while instruction emulation and MMIO will be
> tried for general FAULT page.
>
> The idea comes from Andrew Morton.
>
> Signed-off-by: Huang Ying<ying.huang@intel.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
>
> +#ifdef CONFIG_MEMORY_FAILURE
> +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> +			    unsigned long start, int nr_pages, int write,
> +			    int force, struct page **pages,
> +			    struct vm_area_struct **vmas);
> +#else

Since we'd also like to add get_user_pages_noio(), perhaps adding a 
flags field to get_user_pages() is better.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Huang Ying]

On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> On 01/13/2011 10:42 AM, Huang Ying wrote:
> > Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> > FOLL_HWPOISON is specified.  With this patch, the interested callers
> > can distinguish HWPOISON page from general FAULT page, while other
> > callers will still get -EFAULT for pages, so the user space interface
> > need not to be changed.
> >
> > get_user_pages_hwpoison is added as a variant of get_user_pages that
> > can return -EHWPOISON for HWPOISON page.
> >
> > This feature is needed by KVM, where UCR MCE should be relayed to
> > guest for HWPOISON page, while instruction emulation and MMIO will be
> > tried for general FAULT page.
> >
> > The idea comes from Andrew Morton.
> >
> > Signed-off-by: Huang Ying<ying.huang@intel.com>
> > Cc: Andrew Morton<akpm@linux-foundation.org>
> >
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> > +			    unsigned long start, int nr_pages, int write,
> > +			    int force, struct page **pages,
> > +			    struct vm_area_struct **vmas);
> > +#else
> 
> Since we'd also like to add get_user_pages_noio(), perhaps adding a 
> flags field to get_user_pages() is better.

Or export __get_user_pages()?

Best Regards,
Huang Ying

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Avi Kivity]

On 01/14/2011 03:37 AM, Huang Ying wrote:
> On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> >  On 01/13/2011 10:42 AM, Huang Ying wrote:
> >  >  Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> >  >  FOLL_HWPOISON is specified.  With this patch, the interested callers
> >  >  can distinguish HWPOISON page from general FAULT page, while other
> >  >  callers will still get -EFAULT for pages, so the user space interface
> >  >  need not to be changed.
> >  >
> >  >  get_user_pages_hwpoison is added as a variant of get_user_pages that
> >  >  can return -EHWPOISON for HWPOISON page.
> >  >
> >  >  This feature is needed by KVM, where UCR MCE should be relayed to
> >  >  guest for HWPOISON page, while instruction emulation and MMIO will be
> >  >  tried for general FAULT page.
> >  >
> >  >  The idea comes from Andrew Morton.
> >  >
> >  >  Signed-off-by: Huang Ying<ying.huang@intel.com>
> >  >  Cc: Andrew Morton<akpm@linux-foundation.org>
> >  >
> >  >  +#ifdef CONFIG_MEMORY_FAILURE
> >  >  +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> >  >  +			    unsigned long start, int nr_pages, int write,
> >  >  +			    int force, struct page **pages,
> >  >  +			    struct vm_area_struct **vmas);
> >  >  +#else
> >
> >  Since we'd also like to add get_user_pages_noio(), perhaps adding a
> >  flags field to get_user_pages() is better.
>
> Or export __get_user_pages()?

That's better, yes.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Huang Ying]

Hi, Andrew,

On Sun, 2011-01-16 at 23:35 +0800, Avi Kivity wrote:
> On 01/14/2011 03:37 AM, Huang Ying wrote:
> > On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> > >  On 01/13/2011 10:42 AM, Huang Ying wrote:
> > >  >  Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> > >  >  FOLL_HWPOISON is specified.  With this patch, the interested callers
> > >  >  can distinguish HWPOISON page from general FAULT page, while other
> > >  >  callers will still get -EFAULT for pages, so the user space interface
> > >  >  need not to be changed.
> > >  >
> > >  >  get_user_pages_hwpoison is added as a variant of get_user_pages that
> > >  >  can return -EHWPOISON for HWPOISON page.
> > >  >
> > >  >  This feature is needed by KVM, where UCR MCE should be relayed to
> > >  >  guest for HWPOISON page, while instruction emulation and MMIO will be
> > >  >  tried for general FAULT page.
> > >  >
> > >  >  The idea comes from Andrew Morton.
> > >  >
> > >  >  Signed-off-by: Huang Ying<ying.huang@intel.com>
> > >  >  Cc: Andrew Morton<akpm@linux-foundation.org>
> > >  >
> > >  >  +#ifdef CONFIG_MEMORY_FAILURE
> > >  >  +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> > >  >  +			    unsigned long start, int nr_pages, int write,
> > >  >  +			    int force, struct page **pages,
> > >  >  +			    struct vm_area_struct **vmas);
> > >  >  +#else
> > >
> > >  Since we'd also like to add get_user_pages_noio(), perhaps adding a
> > >  flags field to get_user_pages() is better.
> >
> > Or export __get_user_pages()?
> 
> That's better, yes.

Do you think it is a good idea to export __get_user_pages() instead of
adding get_user_pages_noio() and get_user_pages_hwpoison()?

Best Regards,
Huang Ying

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Marcelo Tosatti]

On Mon, Jan 17, 2011 at 08:47:39AM +0800, Huang Ying wrote:
> Hi, Andrew,
> 
> On Sun, 2011-01-16 at 23:35 +0800, Avi Kivity wrote:
> > On 01/14/2011 03:37 AM, Huang Ying wrote:
> > > On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> > > >  On 01/13/2011 10:42 AM, Huang Ying wrote:
> > > >  >  Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> > > >  >  FOLL_HWPOISON is specified.  With this patch, the interested callers
> > > >  >  can distinguish HWPOISON page from general FAULT page, while other
> > > >  >  callers will still get -EFAULT for pages, so the user space interface
> > > >  >  need not to be changed.
> > > >  >
> > > >  >  get_user_pages_hwpoison is added as a variant of get_user_pages that
> > > >  >  can return -EHWPOISON for HWPOISON page.
> > > >  >
> > > >  >  This feature is needed by KVM, where UCR MCE should be relayed to
> > > >  >  guest for HWPOISON page, while instruction emulation and MMIO will be
> > > >  >  tried for general FAULT page.
> > > >  >
> > > >  >  The idea comes from Andrew Morton.
> > > >  >
> > > >  >  Signed-off-by: Huang Ying<ying.huang@intel.com>
> > > >  >  Cc: Andrew Morton<akpm@linux-foundation.org>
> > > >  >
> > > >  >  +#ifdef CONFIG_MEMORY_FAILURE
> > > >  >  +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> > > >  >  +			    unsigned long start, int nr_pages, int write,
> > > >  >  +			    int force, struct page **pages,
> > > >  >  +			    struct vm_area_struct **vmas);
> > > >  >  +#else
> > > >
> > > >  Since we'd also like to add get_user_pages_noio(), perhaps adding a
> > > >  flags field to get_user_pages() is better.
> > >
> > > Or export __get_user_pages()?
> > 
> > That's better, yes.
> 
> Do you think it is a good idea to export __get_user_pages() instead of
> adding get_user_pages_noio() and get_user_pages_hwpoison()?

Better Andrew and/or Linus should decide it.

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Huang Ying]

Hi, Andrew,

On Thu, 2011-01-20 at 23:50 +0800, Marcelo Tosatti wrote:
> On Mon, Jan 17, 2011 at 08:47:39AM +0800, Huang Ying wrote:
> > Hi, Andrew,
> > 
> > On Sun, 2011-01-16 at 23:35 +0800, Avi Kivity wrote:
> > > On 01/14/2011 03:37 AM, Huang Ying wrote:
> > > > On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> > > > >  On 01/13/2011 10:42 AM, Huang Ying wrote:
> > > > >  >  Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> > > > >  >  FOLL_HWPOISON is specified.  With this patch, the interested callers
> > > > >  >  can distinguish HWPOISON page from general FAULT page, while other
> > > > >  >  callers will still get -EFAULT for pages, so the user space interface
> > > > >  >  need not to be changed.
> > > > >  >
> > > > >  >  get_user_pages_hwpoison is added as a variant of get_user_pages that
> > > > >  >  can return -EHWPOISON for HWPOISON page.
> > > > >  >
> > > > >  >  This feature is needed by KVM, where UCR MCE should be relayed to
> > > > >  >  guest for HWPOISON page, while instruction emulation and MMIO will be
> > > > >  >  tried for general FAULT page.
> > > > >  >
> > > > >  >  The idea comes from Andrew Morton.
> > > > >  >
> > > > >  >  Signed-off-by: Huang Ying<ying.huang@intel.com>
> > > > >  >  Cc: Andrew Morton<akpm@linux-foundation.org>
> > > > >  >
> > > > >  >  +#ifdef CONFIG_MEMORY_FAILURE
> > > > >  >  +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> > > > >  >  +			    unsigned long start, int nr_pages, int write,
> > > > >  >  +			    int force, struct page **pages,
> > > > >  >  +			    struct vm_area_struct **vmas);
> > > > >  >  +#else
> > > > >
> > > > >  Since we'd also like to add get_user_pages_noio(), perhaps adding a
> > > > >  flags field to get_user_pages() is better.
> > > >
> > > > Or export __get_user_pages()?
> > > 
> > > That's better, yes.
> > 
> > Do you think it is a good idea to export __get_user_pages() instead of
> > adding get_user_pages_noio() and get_user_pages_hwpoison()?
> 
> Better Andrew and/or Linus should decide it.

We really need your comments about this.

Best Regards,
Huang Ying

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Andrew Morton]

On Thu, 27 Jan 2011 15:29:05 +0800
Huang Ying <ying.huang@intel.com> wrote:

> Hi, Andrew,
> 
> On Thu, 2011-01-20 at 23:50 +0800, Marcelo Tosatti wrote:
> > On Mon, Jan 17, 2011 at 08:47:39AM +0800, Huang Ying wrote:
> > > Hi, Andrew,
> > > 
> > > On Sun, 2011-01-16 at 23:35 +0800, Avi Kivity wrote:
> > > > On 01/14/2011 03:37 AM, Huang Ying wrote:
> > > > > On Thu, 2011-01-13 at 18:43 +0800, Avi Kivity wrote:
> > > > > >  On 01/13/2011 10:42 AM, Huang Ying wrote:
> > > > > >  >  Make __get_user_pages return -EHWPOISON for HWPOISON page only if
> > > > > >  >  FOLL_HWPOISON is specified.  With this patch, the interested callers
> > > > > >  >  can distinguish HWPOISON page from general FAULT page, while other
> > > > > >  >  callers will still get -EFAULT for pages, so the user space interface
> > > > > >  >  need not to be changed.
> > > > > >  >
> > > > > >  >  get_user_pages_hwpoison is added as a variant of get_user_pages that
> > > > > >  >  can return -EHWPOISON for HWPOISON page.
> > > > > >  >
> > > > > >  >  This feature is needed by KVM, where UCR MCE should be relayed to
> > > > > >  >  guest for HWPOISON page, while instruction emulation and MMIO will be
> > > > > >  >  tried for general FAULT page.
> > > > > >  >
> > > > > >  >  The idea comes from Andrew Morton.
> > > > > >  >
> > > > > >  >  Signed-off-by: Huang Ying<ying.huang@intel.com>
> > > > > >  >  Cc: Andrew Morton<akpm@linux-foundation.org>
> > > > > >  >
> > > > > >  >  +#ifdef CONFIG_MEMORY_FAILURE
> > > > > >  >  +int get_user_pages_hwpoison(struct task_struct *tsk, struct mm_struct *mm,
> > > > > >  >  +			    unsigned long start, int nr_pages, int write,
> > > > > >  >  +			    int force, struct page **pages,
> > > > > >  >  +			    struct vm_area_struct **vmas);
> > > > > >  >  +#else
> > > > > >
> > > > > >  Since we'd also like to add get_user_pages_noio(), perhaps adding a
> > > > > >  flags field to get_user_pages() is better.
> > > > >
> > > > > Or export __get_user_pages()?
> > > > 
> > > > That's better, yes.
> > > 
> > > Do you think it is a good idea to export __get_user_pages() instead of
> > > adding get_user_pages_noio() and get_user_pages_hwpoison()?
> > 
> > Better Andrew and/or Linus should decide it.
> 
> We really need your comments about this.
> 

Export __get_user_pages(), I guess.  We don't need hwpoison/kvm-specific
stuff in mm/memory.c and
get_user_pages_hwpoison()/get_user_pages_noio() are very thin
wrappers.

The number of args to these functions is getting nutty - you'll
probably find that it is beneficial to inline these wrapepr functions, if
the number of callsites is small.

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Andi Kleen]

I personally would consider it cleaner to have clearly
defined wrappers instead of complicted flags in the caller.

> The number of args to these functions is getting nutty - you'll
> probably find that it is beneficial to inline these wrapepr functions, if
> the number of callsites is small.

Really the functions are so heavy weight it should not matter.
The problem with inlining is that you end up with the code in
the header file and I personally find that much harder to browse
instead of having everything in one file.

Also longer term we'll get compilers that can do cross-file inlining
for optimized builds.

So please better avoid these kinds of micro optimizations unless
it's a really really extremly speed critical path.

Thanks,

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Andrew Morton]

On Fri, 28 Jan 2011 01:57:11 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> I personally would consider it cleaner to have clearly
> defined wrappers instead of complicted flags in the caller.
> 
> > The number of args to these functions is getting nutty - you'll
> > probably find that it is beneficial to inline these wrapepr functions, if
> > the number of callsites is small.
> 
> Really the functions are so heavy weight it should not matter.
> The problem with inlining is that you end up with the code in
> the header file and I personally find that much harder to browse
> instead of having everything in one file.

You'll cope.

> Also longer term we'll get compilers that can do cross-file inlining
> for optimized builds.

Which we'll probably need to turn off all over the place :(

> So please better avoid these kinds of micro optimizations unless
> it's a really really extremly speed critical path.

It's not just speed and it's not just .text size, either. Calling a
ten-arg function consumes stack space.

Re: [PATCH 1/2] mm, Make __get_user_pages return -EHWPOISON for HWPOISON page optionally

[Andi Kleen]

> > Also longer term we'll get compilers that can do cross-file inlining
> > for optimized builds.
> 
> Which we'll probably need to turn off all over the place :(

Why? 

> 
> > So please better avoid these kinds of micro optimizations unless
> > it's a really really extremly speed critical path.
> 
> It's not just speed and it's not just .text size, either. Calling a
> ten-arg function consumes stack space.

gcc is pretty good at handling tail calls. Take a look at the code
it generates.


-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.