[PATCH 1/2] mm: Make follow_pte_pmd an inline

[PATCH 1/2] mm: Make follow_pte_pmd an inline

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

The one user of follow_pte_pmd (dax) emits a sparse warning because
it doesn't know that follow_pte_pmd conditionally returns with the
pte/pmd locked.  The required annotation is already there; it's just
in the wrong file.
---
 include/linux/mm.h | 15 ++++++++++++++-
 mm/memory.c        | 16 +---------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ea818ff739cd..94a9d2149bd6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1314,7 +1314,7 @@ int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
 			struct vm_area_struct *vma);
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows);
-int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
+int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 			     unsigned long *start, unsigned long *end,
 			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
 int follow_pfn(struct vm_area_struct *vma, unsigned long address,
@@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address,
 int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
 			void *buf, int len, int write);
 
+static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
+			     unsigned long *start, unsigned long *end,
+			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
+{
+	int res;
+
+	/* (void) is needed to make gcc happy */
+	(void) __cond_lock(*ptlp,
+			   !(res = __follow_pte_pmd(mm, address, start, end,
+						    ptepp, pmdpp, ptlp)));
+	return res;
+}
+
 static inline void unmap_shared_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen)
 {
diff --git a/mm/memory.c b/mm/memory.c
index cfaba6287702..cb433662af21 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4201,7 +4201,7 @@ int __pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
 }
 #endif /* __PAGETABLE_PMD_FOLDED */
 
-static int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
+int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 			    unsigned long *start, unsigned long *end,
 			    pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
 {
@@ -4278,20 +4278,6 @@ static inline int follow_pte(struct mm_struct *mm, unsigned long address,
 	return res;
 }
 
-int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
-			     unsigned long *start, unsigned long *end,
-			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
-{
-	int res;
-
-	/* (void) is needed to make gcc happy */
-	(void) __cond_lock(*ptlp,
-			   !(res = __follow_pte_pmd(mm, address, start, end,
-						    ptepp, pmdpp, ptlp)));
-	return res;
-}
-EXPORT_SYMBOL(follow_pte_pmd);
-
 /**
  * follow_pfn - look up PFN at a user virtual address
  * @vma: memory mapping
-- 
2.15.1

[PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

The __cond_lock macro expects the function to return 'true' if the lock
was acquired and 'false' if it wasn't.  We have another common calling
convention in the kernel, which is returning 0 on success and an errno
on failure.  It's hard to use the existing __cond_lock macro for those
kinds of functions, so introduce __cond_lock_err() and convert the
two existing users.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 include/linux/compiler_types.h | 2 ++
 include/linux/mm.h             | 9 ++-------
 mm/memory.c                    | 9 ++-------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6b79a9bba9a7..ff3c41c78efa 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -16,6 +16,7 @@
 # define __acquire(x)	__context__(x,1)
 # define __release(x)	__context__(x,-1)
 # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
+# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
 # define __percpu	__attribute__((noderef, address_space(3)))
 # define __rcu		__attribute__((noderef, address_space(4)))
 # define __private	__attribute__((noderef))
@@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 # define __acquire(x) (void)0
 # define __release(x) (void)0
 # define __cond_lock(x,c) (c)
+# define __cond_lock_err(x,c) (c)
 # define __percpu
 # define __rcu
 # define __private
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 94a9d2149bd6..2ccdc980296b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 			     unsigned long *start, unsigned long *end,
 			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
 {
-	int res;
-
-	/* (void) is needed to make gcc happy */
-	(void) __cond_lock(*ptlp,
-			   !(res = __follow_pte_pmd(mm, address, start, end,
-						    ptepp, pmdpp, ptlp)));
-	return res;
+	return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end,
+						    ptepp, pmdpp, ptlp));
 }
 
 static inline void unmap_shared_mapping_range(struct address_space *mapping,
diff --git a/mm/memory.c b/mm/memory.c
index cb433662af21..92d58309cf45 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 static inline int follow_pte(struct mm_struct *mm, unsigned long address,
 			     pte_t **ptepp, spinlock_t **ptlp)
 {
-	int res;
-
-	/* (void) is needed to make gcc happy */
-	(void) __cond_lock(*ptlp,
-			   !(res = __follow_pte_pmd(mm, address, NULL, NULL,
-						    ptepp, NULL, ptlp)));
-	return res;
+	return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL,
+						    ptepp, NULL, ptlp));
 }
 
 /**
-- 
2.15.1

Re: [PATCH 2/2] Introduce __cond_lock_err

[Ross Zwisler]

On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> The __cond_lock macro expects the function to return 'true' if the lock
> was acquired and 'false' if it wasn't.  We have another common calling
> convention in the kernel, which is returning 0 on success and an errno
> on failure.  It's hard to use the existing __cond_lock macro for those
> kinds of functions, so introduce __cond_lock_err() and convert the
> two existing users.

This is much cleaner!  One quick issue below.

> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  include/linux/compiler_types.h | 2 ++
>  include/linux/mm.h             | 9 ++-------
>  mm/memory.c                    | 9 ++-------
>  3 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6b79a9bba9a7..ff3c41c78efa 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -16,6 +16,7 @@
>  # define __acquire(x)	__context__(x,1)
>  # define __release(x)	__context__(x,-1)
>  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
> +# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
					       ^
I think we actually want this to return c here ^

The old code saved off the actual return value from __follow_pte_pmd() (say,
-EINVAL) in 'res', and that was what was returned on error from both
follow_pte_pmd() and follow_pte().  The value of 1 returned by __cond_lock()
was just discarded (after we cast it to void for some reason).

With this new code we actually return the value from __cond_lock_err(), which
means that instead of returning -EINVAL, we'll return 1 on error.

>  # define __percpu	__attribute__((noderef, address_space(3)))
>  # define __rcu		__attribute__((noderef, address_space(4)))
>  # define __private	__attribute__((noderef))
> @@ -42,6 +43,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>  # define __acquire(x) (void)0
>  # define __release(x) (void)0
>  # define __cond_lock(x,c) (c)
> +# define __cond_lock_err(x,c) (c)
>  # define __percpu
>  # define __rcu
>  # define __private
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 94a9d2149bd6..2ccdc980296b 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1328,13 +1328,8 @@ static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
>  			     unsigned long *start, unsigned long *end,
>  			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
>  {
> -	int res;
> -
> -	/* (void) is needed to make gcc happy */
> -	(void) __cond_lock(*ptlp,
> -			   !(res = __follow_pte_pmd(mm, address, start, end,
> -						    ptepp, pmdpp, ptlp)));
> -	return res;
> +	return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end,
> +						    ptepp, pmdpp, ptlp));
>  }
>  
>  static inline void unmap_shared_mapping_range(struct address_space *mapping,
> diff --git a/mm/memory.c b/mm/memory.c
> index cb433662af21..92d58309cf45 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4269,13 +4269,8 @@ int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
>  static inline int follow_pte(struct mm_struct *mm, unsigned long address,
>  			     pte_t **ptepp, spinlock_t **ptlp)
>  {
> -	int res;
> -
> -	/* (void) is needed to make gcc happy */
> -	(void) __cond_lock(*ptlp,
> -			   !(res = __follow_pte_pmd(mm, address, NULL, NULL,
> -						    ptepp, NULL, ptlp)));
> -	return res;
> +	return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, NULL, NULL,
> +						    ptepp, NULL, ptlp));
>  }
>  
>  /**
> -- 
> 2.15.1
> 

Re: [PATCH 2/2] Introduce __cond_lock_err

[Josh Triplett]

On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote:
> On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > The __cond_lock macro expects the function to return 'true' if the lock
> > was acquired and 'false' if it wasn't.  We have another common calling
> > convention in the kernel, which is returning 0 on success and an errno
> > on failure.  It's hard to use the existing __cond_lock macro for those
> > kinds of functions, so introduce __cond_lock_err() and convert the
> > two existing users.
> 
> This is much cleaner!  One quick issue below.
> 
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > ---
> >  include/linux/compiler_types.h | 2 ++
> >  include/linux/mm.h             | 9 ++-------
> >  mm/memory.c                    | 9 ++-------
> >  3 files changed, 6 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 6b79a9bba9a7..ff3c41c78efa 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -16,6 +16,7 @@
> >  # define __acquire(x)	__context__(x,1)
> >  # define __release(x)	__context__(x,-1)
> >  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
> > +# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
> 					       ^
> I think we actually want this to return c here ^

Then you want to use ((c) ?: ...), to avoid evaluating c twice.

- Josh Triplett

Re: [PATCH 2/2] Introduce __cond_lock_err

[Ross Zwisler]

On Thu, Dec 21, 2017 at 02:00:16PM -0800, Josh Triplett wrote:
> On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote:
> > On Tue, Dec 19, 2017 at 08:58:23AM -0800, Matthew Wilcox wrote:
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > The __cond_lock macro expects the function to return 'true' if the lock
> > > was acquired and 'false' if it wasn't.  We have another common calling
> > > convention in the kernel, which is returning 0 on success and an errno
> > > on failure.  It's hard to use the existing __cond_lock macro for those
> > > kinds of functions, so introduce __cond_lock_err() and convert the
> > > two existing users.
> > 
> > This is much cleaner!  One quick issue below.
> > 
> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > ---
> > >  include/linux/compiler_types.h | 2 ++
> > >  include/linux/mm.h             | 9 ++-------
> > >  mm/memory.c                    | 9 ++-------
> > >  3 files changed, 6 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > > index 6b79a9bba9a7..ff3c41c78efa 100644
> > > --- a/include/linux/compiler_types.h
> > > +++ b/include/linux/compiler_types.h
> > > @@ -16,6 +16,7 @@
> > >  # define __acquire(x)	__context__(x,1)
> > >  # define __release(x)	__context__(x,-1)
> > >  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
> > > +# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
> > 					       ^
> > I think we actually want this to return c here ^
> 
> Then you want to use ((c) ?: ...), to avoid evaluating c twice.

Oh, yep, great catch.

Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote:
> > +++ b/include/linux/compiler_types.h
> > @@ -16,6 +16,7 @@
> >  # define __acquire(x)	__context__(x,1)
> >  # define __release(x)	__context__(x,-1)
> >  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
> > +# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
> 					       ^
> I think we actually want this to return c here ^
> 
> The old code saved off the actual return value from __follow_pte_pmd() (say,
> -EINVAL) in 'res', and that was what was returned on error from both
> follow_pte_pmd() and follow_pte().  The value of 1 returned by __cond_lock()
> was just discarded (after we cast it to void for some reason).
> 
> With this new code we actually return the value from __cond_lock_err(), which
> means that instead of returning -EINVAL, we'll return 1 on error.

Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
return as this code will never run.

That said, if sparse supports the GNU syntax of ?: then I have no
objection to doing that.

Re: [PATCH 2/2] Introduce __cond_lock_err

[Josh Triplett]

On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote:
> On Thu, Dec 21, 2017 at 02:48:10PM -0700, Ross Zwisler wrote:
> > > +++ b/include/linux/compiler_types.h
> > > @@ -16,6 +16,7 @@
> > >  # define __acquire(x)	__context__(x,1)
> > >  # define __release(x)	__context__(x,-1)
> > >  # define __cond_lock(x,c)	((c) ? ({ __acquire(x); 1; }) : 0)
> > > +# define __cond_lock_err(x,c)	((c) ? 1 : ({ __acquire(x); 0; }))
> > 					       ^
> > I think we actually want this to return c here ^
> > 
> > The old code saved off the actual return value from __follow_pte_pmd() (say,
> > -EINVAL) in 'res', and that was what was returned on error from both
> > follow_pte_pmd() and follow_pte().  The value of 1 returned by __cond_lock()
> > was just discarded (after we cast it to void for some reason).
> > 
> > With this new code we actually return the value from __cond_lock_err(), which
> > means that instead of returning -EINVAL, we'll return 1 on error.
> 
> Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
> return as this code will never run.

It does matter slightly, as Sparse does some (very limited) value-based
analyses. Let's future-proof it.

> That said, if sparse supports the GNU syntax of ?: then I have no
> objection to doing that.

Sparse does support that syntax.

- Josh Triplett

Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote:
> > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
> > return as this code will never run.
> 
> It does matter slightly, as Sparse does some (very limited) value-based
> analyses. Let's future-proof it.
> 
> > That said, if sparse supports the GNU syntax of ?: then I have no
> > objection to doing that.
> 
> Sparse does support that syntax.

Great, I'll fix that and resubmit.

While I've got you, I've been looking at some other sparse warnings from
this file.  There are several caused by sparse being unable to handle
the following construct:

	if (foo)
		x = NULL;
	else {
		x = bar;
		__acquire(bar);
	}
	if (!x)
		return -ENOMEM;

Writing it as:

	if (foo)
		return -ENOMEM;
	else {
		x = bar;
		__acquire(bar);
	}

works just fine.  ie this removes the warning:

@@ -1070,9 +1070,9 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct
 mm_struct *src_mm,
 again:
        init_rss_vec(rss);
 
-       dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
-       if (!dst_pte)
+       if (pte_alloc(dst_mm, dst_pmd, addr))
                return -ENOMEM;
+       dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
        src_pte = pte_offset_map(src_pmd, addr);
        src_ptl = pte_lockptr(src_mm, src_pmd);
        spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);

Is there any chance sparse's dataflow analysis will be improved in the
near future?

Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote:
> > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
> > > return as this code will never run.
> > 
> > It does matter slightly, as Sparse does some (very limited) value-based
> > analyses. Let's future-proof it.
> > 
> > > That said, if sparse supports the GNU syntax of ?: then I have no
> > > objection to doing that.
> > 
> > Sparse does support that syntax.
> 
> Great, I'll fix that and resubmit.

Except the context imbalance warning comes back if I do.  This is sparse
0.5.1 (Debian's 0.5.1-2 package).

Re: [PATCH 2/2] Introduce __cond_lock_err

[Josh Triplett]

+linux-sparse

On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote:
> On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote:
> > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
> > > > return as this code will never run.
> > > 
> > > It does matter slightly, as Sparse does some (very limited) value-based
> > > analyses. Let's future-proof it.
> > > 
> > > > That said, if sparse supports the GNU syntax of ?: then I have no
> > > > objection to doing that.
> > > 
> > > Sparse does support that syntax.
> > 
> > Great, I'll fix that and resubmit.
> 
> Except the context imbalance warning comes back if I do.  This is sparse
> 0.5.1 (Debian's 0.5.1-2 package).

Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote:
> +linux-sparse

Ehh ... we've probably trimmed too much to give linux-sparse a good summary.

Here're the important lines from my patch:

+# define __cond_lock_err(x,c)  ((c) ? 1 : ({ __acquire(x); 0; }))

+       return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end,
+                                                   ptepp, pmdpp, ptlp));

This is supposed to be "If "c" is an error value, we don't have a lock,
otherwise we have a lock".  And to translate from linux-speak into
sparse-speak:

 # define __acquire(x)  __context__(x,1)

Josh & Ross pointed out (quite correctly) that code which does something like

if (foo())
	return;

will work with this, but code that does

if (foo() < 0)
	return;

will not because we're now returning 1 instead of -ENOMEM (for example).

So they made the very sensible suggestion that I change the definition
of __cond_lock to:

# define __cond_lock_err(x,c)  ((c) ?: ({ __acquire(x); 0; }))

Unfortunately, when I do that, the context imbalance warning returns.
As I said below, this is with sparse 0.5.1.

> On Fri, Dec 22, 2017 at 05:36:34AM -0800, Matthew Wilcox wrote:
> > On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> > > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > > > On Thu, Dec 21, 2017 at 05:10:00PM -0800, Matthew Wilcox wrote:
> > > > > Yes, but this define is only #if __CHECKER__, so it doesn't matter what we
> > > > > return as this code will never run.
> > > > 
> > > > It does matter slightly, as Sparse does some (very limited) value-based
> > > > analyses. Let's future-proof it.
> > > > 
> > > > > That said, if sparse supports the GNU syntax of ?: then I have no
> > > > > objection to doing that.
> > > > 
> > > > Sparse does support that syntax.
> > > 
> > > Great, I'll fix that and resubmit.
> > 
> > Except the context imbalance warning comes back if I do.  This is sparse
> > 0.5.1 (Debian's 0.5.1-2 package).
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] Introduce __cond_lock_err

[Luc Van Oostenryck]

On Sat, Dec 23, 2017 at 05:06:21AM -0800, Matthew Wilcox wrote:
> On Sat, Dec 23, 2017 at 01:39:11AM -0800, Josh Triplett wrote:
> > +linux-sparse
> 
> Ehh ... we've probably trimmed too much to give linux-sparse a good summary.
> 
> Here're the important lines from my patch:
> 
> +# define __cond_lock_err(x,c)  ((c) ? 1 : ({ __acquire(x); 0; }))
> 
> +       return __cond_lock_err(*ptlp, __follow_pte_pmd(mm, address, start, end,
> +                                                   ptepp, pmdpp, ptlp));
> 
> This is supposed to be "If "c" is an error value, we don't have a lock,
> otherwise we have a lock".  And to translate from linux-speak into
> sparse-speak:
> 
>  # define __acquire(x)  __context__(x,1)
> 
> Josh & Ross pointed out (quite correctly) that code which does something like
> 
> if (foo())
> 	return;
> 
> will work with this, but code that does
> 
> if (foo() < 0)
> 	return;
> 
> will not because we're now returning 1 instead of -ENOMEM (for example).
> 
> So they made the very sensible suggestion that I change the definition
> of __cond_lock to:
> 
> # define __cond_lock_err(x,c)  ((c) ?: ({ __acquire(x); 0; }))
> 
> Unfortunately, when I do that, the context imbalance warning returns.
> As I said below, this is with sparse 0.5.1.

I think this __cond_lock_err() is now OK (but some comment about
how its use is different from __cond_lock() would be welcome).

For the context imbalance, I would really need a concrete example
to be able to help more because it depends heavily on what the
test is and what code is before and after.

If you can point me to a tree, a .config and a specific warning,
I'll be glad to take a look.

-- Luc Van Oostenryck

Re: [PATCH 2/2] Introduce __cond_lock_err

[Luc Van Oostenryck]

On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> 
> While I've got you, I've been looking at some other sparse warnings from
> this file.  There are several caused by sparse being unable to handle
> the following construct:
> 
> 	if (foo)
> 		x = NULL;
> 	else {
> 		x = bar;
> 		__acquire(bar);
> 	}
> 	if (!x)
> 		return -ENOMEM;
> 
> Writing it as:
> 
> 	if (foo)
> 		return -ENOMEM;
> 	else {
> 		x = bar;
> 		__acquire(bar);
> 	}
> 
> works just fine.  ie this removes the warning:

It must be noted that these two versions are not equivalent
(in the first version, it also returns with -ENOMEM if bar
is NULL/zero).
 
It must be noted that sparse's goal regarding the context imbalance
is to give the warning if some point in the code can be reached via
two paths (or more) and the lock state (the context) is not identical
in each of these paths.

> 
> Is there any chance sparse's dataflow analysis will be improved in the
> near future?

A lot of functions in the kernel have this context imbalance,
really a lot. For example, any function doing conditional locking
is a problem here. Happily when these functions are inlined,
sparse, thanks to its optimizations, can remove some paths and
merge some others. 
So yes, by adding some smartness to sparse, some of the false
warnings will be removed, however:
1) some __must_hold()/__acquires()/__releases() annotations are
   missing, making sparse's job impossible.
2) a lot of the 'false warnings' are not so false because there is
   indeed two possible paths with different lock state
3) it has its limits (at the end, giving the correct warning is
   equivalent to the halting problem).

Now, to answer to your question, I'm not aware of any effort that would
make a significant differences (it would need, IMO, code hoisting & 
value range propagation).

-- Luc Van Oostenryck

Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox]

On Wed, Dec 27, 2017 at 03:28:54PM +0100, Luc Van Oostenryck wrote:
> On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > 
> > While I've got you, I've been looking at some other sparse warnings from
> > this file.  There are several caused by sparse being unable to handle
> > the following construct:
> > 
> > 	if (foo)
> > 		x = NULL;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 	if (!x)
> > 		return -ENOMEM;
> > 
> > Writing it as:
> > 
> > 	if (foo)
> > 		return -ENOMEM;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 
> > works just fine.  ie this removes the warning:
> 
> It must be noted that these two versions are not equivalent
> (in the first version, it also returns with -ENOMEM if bar
> is NULL/zero).

They happen to be equivalent in the original; I was providing a simplified
version.  Here's the construct sparse can't understand:

        dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
        if (!dst_pte)
                return -ENOMEM;

with:

#define pte_alloc(mm, pmd, address)                     \
        (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, address))

#define pte_offset_map_lock(mm, pmd, address, ptlp)     \
({                                                      \
        spinlock_t *__ptl = pte_lockptr(mm, pmd);       \
        pte_t *__pte = pte_offset_map(pmd, address);    \
        *(ptlp) = __ptl;                                \
        spin_lock(__ptl);                               \
        __pte;                                          \
})

#define pte_alloc_map_lock(mm, pmd, address, ptlp)      \
        (pte_alloc(mm, pmd, address) ?                  \
                 NULL : pte_offset_map_lock(mm, pmd, address, ptlp))

If pte_alloc() succeeds, pte_offset_map_lock() will return non-NULL.
Manually inlining pte_alloc_map_lock() into the caller like so:

        if (pte_alloc(dst_mm, dst_pmd, addr)
		return -ENOMEM;
        dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, ptlp);

causes sparse to not warn.

> > Is there any chance sparse's dataflow analysis will be improved in the
> > near future?
> 
> A lot of functions in the kernel have this context imbalance,
> really a lot. For example, any function doing conditional locking
> is a problem here. Happily when these functions are inlined,
> sparse, thanks to its optimizations, can remove some paths and
> merge some others. 
> So yes, by adding some smartness to sparse, some of the false
> warnings will be removed, however:
> 1) some __must_hold()/__acquires()/__releases() annotations are
>    missing, making sparse's job impossible.

Partly there's a documentation problem here.  I'd really like to see a
document explaining how to add sparse annotations to a function which
intentionally does conditional locking.  For example, should we be
annotating the function as __acquires, and then marking the exits which
don't acquire the lock with __acquire(), or should we not annotate
the function, and annotate the exits which _do_ acquire the lock as
__release() with a comment like /* Caller will release */

> 2) a lot of the 'false warnings' are not so false because there is
>    indeed two possible paths with different lock state
> 3) it has its limits (at the end, giving the correct warning is
>    equivalent to the halting problem).
> 
> Now, to answer to your question, I'm not aware of any effort that would
> make a significant differences (it would need, IMO, code hoisting & 
> value range propagation).

That's fair.  I wonder if we were starting from scratch whether we'd
choose to make sparse a GCC plugin today.

Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline

[Joe Perches]

On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> The one user of follow_pte_pmd (dax) emits a sparse warning because
> it doesn't know that follow_pte_pmd conditionally returns with the
> pte/pmd locked.  The required annotation is already there; it's just
> in the wrong file.
[]
> diff --git a/include/linux/mm.h b/include/linux/mm.h
[]
> @@ -1324,6 +1324,19 @@ int follow_phys(struct vm_area_struct *vma, unsigned long address,
>  int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>  			void *buf, int len, int write);
>  
> +static inline int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
> +			     unsigned long *start, unsigned long *end,
> +			     pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp)
> +{
> +	int res;
> +
> +	/* (void) is needed to make gcc happy */
> +	(void) __cond_lock(*ptlp,
> +			   !(res = __follow_pte_pmd(mm, address, start, end,
> +						    ptepp, pmdpp, ptlp)));

This seems obscure and difficult to read.  Perhaps:

	res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp);
	(void)__cond_lock(*ptlp, !res);

> +	return res;
> +}

Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline

[Matthew Wilcox]

On Tue, Dec 19, 2017 at 09:05:42AM -0800, Joe Perches wrote:
> On Tue, 2017-12-19 at 08:58 -0800, Matthew Wilcox wrote:
> > +	/* (void) is needed to make gcc happy */
> > +	(void) __cond_lock(*ptlp,
> > +			   !(res = __follow_pte_pmd(mm, address, start, end,
> > +						    ptepp, pmdpp, ptlp)));
> 
> This seems obscure and difficult to read.  Perhaps:
> 
> 	res = __follow_pte_pmd(mm, address, start, end, ptepp, pmdpp, ptlp);
> 	(void)__cond_lock(*ptlp, !res);

Patch 1 moves the code.  Patch 2 cleans it up ;-)

Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline

[Ross Zwisler]

On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> The one user of follow_pte_pmd (dax) emits a sparse warning because
> it doesn't know that follow_pte_pmd conditionally returns with the
> pte/pmd locked.  The required annotation is already there; it's just
> in the wrong file.

Can you help me find the required annotation that is already there but in the
wrong file?

This does seem to quiet a lockep warning in fs/dax.c, but I think we still
have a related one in mm/memory.c:

mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block

Should we deal with this one as well?

Re: [PATCH 1/2] mm: Make follow_pte_pmd an inline

[Matthew Wilcox]

On Thu, Dec 21, 2017 at 02:29:43PM -0700, Ross Zwisler wrote:
> On Tue, Dec 19, 2017 at 08:58:22AM -0800, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > The one user of follow_pte_pmd (dax) emits a sparse warning because
> > it doesn't know that follow_pte_pmd conditionally returns with the
> > pte/pmd locked.  The required annotation is already there; it's just
> > in the wrong file.
> 
> Can you help me find the required annotation that is already there but in the
> wrong file?

You cut it out ... that was the entire contents of the patch!
The cond_lock annotation is correct, but sparse doesn't look across
compilation units, so it can't see the one that's in mm/memory.c when
it's compiling fs/dax.c.  That's why it needs to be in a header file.

> This does seem to quiet a lockep warning in fs/dax.c, but I think we still
> have a related one in mm/memory.c:
> 
> mm/memory.c:4204:5: warning: context imbalance in '__follow_pte_pmd' - different lock contexts for basic block
> 
> Should we deal with this one as well?

I'm not sure how to deal with that one, to be honest.