find_swap_entry sparse cleanup

find_swap_entry sparse cleanup

[Mike Kravetz]

As part of restructuring code for memfd, I want clean up all the
sparse warnings in mm/shmem.c.  Most are straight forward, but I
am not sure about find_swap_entry.  Specifically the code:

	rcu_read_lock();
	radix_tree_for_each_slot(slot, root, &iter, 0) {
		if (*slot == item) {
			found = iter.index;
			break;
		}
		checked++;
		if ((checked % 4096) != 0)
			continue;
		slot = radix_tree_iter_resume(slot, &iter);
		cond_resched_rcu();
	}
	rcu_read_unlock();

The complaint is about that (*slot == item) comparison.

My first thought was to do the radix_tree_deref_slot(),
radix_tree_exception(), radix_tree_deref_retry() thing.
However, I was concerned that swap entries (which this routine
is looking for) are stored as exception entries?  So, perhaps
this should just use rcu_dereference_raw()?

Suggestions would be appreciated.

And, yes I do know that the XArray code would replace all this.
-- 
Mike Kravetz

Re: find_swap_entry sparse cleanup

[Matthew Wilcox]

On Fri, Apr 06, 2018 at 03:13:45PM -0700, Mike Kravetz wrote:
> As part of restructuring code for memfd, I want clean up all the
> sparse warnings in mm/shmem.c.  Most are straight forward, but I
> am not sure about find_swap_entry.  Specifically the code:
> 
> 	rcu_read_lock();
> 	radix_tree_for_each_slot(slot, root, &iter, 0) {
> 		if (*slot == item) {
> 			found = iter.index;
> 			break;
> 		}
> 		checked++;
> 		if ((checked % 4096) != 0)
> 			continue;
> 		slot = radix_tree_iter_resume(slot, &iter);
> 		cond_resched_rcu();
> 	}
> 	rcu_read_unlock();
> 
> The complaint is about that (*slot == item) comparison.
> 
> My first thought was to do the radix_tree_deref_slot(),
> radix_tree_exception(), radix_tree_deref_retry() thing.
> However, I was concerned that swap entries (which this routine
> is looking for) are stored as exception entries?  So, perhaps
> this should just use rcu_dereference_raw()?
> 
> Suggestions would be appreciated.
> 
> And, yes I do know that the XArray code would replace all this.

I'm happy to help clean this up in advance of the XArray code going in ...

This loop is actually buggy in two or three different ways.  Here's how
it should have looked:

@@ -1098,13 +1098,18 @@ static void shmem_evict_inode(struct inode *inode)
 static unsigned long find_swap_entry(struct radix_tree_root *root, void *item)
 {
        struct radix_tree_iter iter;
-       void **slot;
+       void __rcu **slot;
        unsigned long found = -1;
        unsigned int checked = 0;
 
        rcu_read_lock();
        radix_tree_for_each_slot(slot, root, &iter, 0) {
-               if (*slot == item) {
+               void *entry = radix_tree_deref_slot(slot);
+               if (radix_tree_deref_retry(entry)) {
+                       slot = radix_tree_iter_retry(&iter);
+                       continue;
+               }
+               if (entry == item) {
                        found = iter.index;
                        break;
                }

Re: find_swap_entry sparse cleanup

[Mike Kravetz]

On 04/06/2018 07:28 PM, Matthew Wilcox wrote:
> I'm happy to help clean this up in advance of the XArray code going in ...
> 
> This loop is actually buggy in two or three different ways.  Here's how
> it should have looked:
> 
> @@ -1098,13 +1098,18 @@ static void shmem_evict_inode(struct inode *inode)
>  static unsigned long find_swap_entry(struct radix_tree_root *root, void *item)
>  {
>         struct radix_tree_iter iter;
> -       void **slot;
> +       void __rcu **slot;
>         unsigned long found = -1;
>         unsigned int checked = 0;
>  
>         rcu_read_lock();
>         radix_tree_for_each_slot(slot, root, &iter, 0) {
> -               if (*slot == item) {
> +               void *entry = radix_tree_deref_slot(slot);
> +               if (radix_tree_deref_retry(entry)) {
> +                       slot = radix_tree_iter_retry(&iter);
> +                       continue;
> +               }
> +               if (entry == item) {
>                         found = iter.index;
>                         break;
>                 }
> 

Thank you!  I was worried about searching for swap entries that would
be marked RADIX_TREE_EXCEPTIONAL_ENTRY.  Your changes above make perfect
sense.  Do you mind if I roll them into a patch that adds all the other
missing __rcu annotations in the file, and add your Signed-off-by?

-- 
Mike Kravetz