Re: [PATCH v10 43/62] memfd: Convert shmem_tag_pins to XArray

[Mike Kravetz <mike.kravetz@oracle.com>]

On 03/29/2018 08:42 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Simplify the locking by taking the spinlock while we walk the tree on
> the assumption that many acquires and releases of the lock will be
> worse than holding the lock for a (potentially) long time.

I see this change made in several of the patches and do not have a
specific issue with it.  As part of the XArray implementation you
have XA_CHECK_SCHED = 4096.   So, we drop locks and do a cond_resched
after XA_CHECK_SCHED iterations.  Just curious how you came up with
that number.  Apologies in advance if this was discussed in a previous
round of reviews.

> 
> We could replicate the same locking behaviour with the xarray, but would
> have to be careful that the xa_node wasn't RCU-freed under us before we
> took the lock.
> 
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

I did not do a detailed review of the XArray implementation.  Only looked
at the provided interfaces and their intended uses.  If the interfaces work
as specified, the changes here are fine.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> ---
>  mm/memfd.c | 43 ++++++++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 4cf7401cb09c..3b299d72df78 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -21,7 +21,7 @@
>  #include <uapi/linux/memfd.h>
>  
>  /*
> - * We need a tag: a new tag would expand every radix_tree_node by 8 bytes,
> + * We need a tag: a new tag would expand every xa_node by 8 bytes,
>   * so reuse a tag which we firmly believe is never set or cleared on shmem.
>   */
>  #define SHMEM_TAG_PINNED        PAGECACHE_TAG_TOWRITE
> @@ -29,35 +29,28 @@
>  
>  static void shmem_tag_pins(struct address_space *mapping)
>  {
> -	struct radix_tree_iter iter;
> -	void __rcu **slot;
> -	pgoff_t start;
> +	XA_STATE(xas, &mapping->i_pages, 0);
>  	struct page *page;
> +	unsigned int tagged = 0;
>  
>  	lru_add_drain();
> -	start = 0;
> -	rcu_read_lock();
> -
> -	radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) {
> -		page = radix_tree_deref_slot(slot);
> -		if (!page || radix_tree_exception(page)) {
> -			if (radix_tree_deref_retry(page)) {
> -				slot = radix_tree_iter_retry(&iter);
> -				continue;
> -			}
> -		} else if (page_count(page) - page_mapcount(page) > 1) {
> -			xa_lock_irq(&mapping->i_pages);
> -			radix_tree_tag_set(&mapping->i_pages, iter.index,
> -					   SHMEM_TAG_PINNED);
> -			xa_unlock_irq(&mapping->i_pages);
> -		}
>  
> -		if (need_resched()) {
> -			slot = radix_tree_iter_resume(slot, &iter);
> -			cond_resched_rcu();
> -		}
> +	xas_lock_irq(&xas);
> +	xas_for_each(&xas, page, ULONG_MAX) {
> +		if (xa_is_value(page))
> +			continue;
> +		if (page_count(page) - page_mapcount(page) > 1)
> +			xas_set_tag(&xas, SHMEM_TAG_PINNED);
> +
> +		if (++tagged % XA_CHECK_SCHED)
> +			continue;
> +
> +		xas_pause(&xas);
> +		xas_unlock_irq(&xas);
> +		cond_resched();
> +		xas_lock_irq(&xas);
>  	}
> -	rcu_read_unlock();
> +	xas_unlock_irq(&xas);
>  }
>  
>  /*
>