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;
}
