From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Kravetz Subject: Re: [PATCH v10 43/62] memfd: Convert shmem_tag_pins to XArray Date: Fri, 30 Mar 2018 17:05:05 -0700 Message-ID: <39ea3393-c3d7-07c3-a072-344f3a65cef3@oracle.com> References: <20180330034245.10462-1-willy@infradead.org> <20180330034245.10462-44-willy@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1f24T7-0007sV-6z for linux-f2fs-devel@lists.sourceforge.net; Sat, 31 Mar 2018 00:34:05 +0000 Received: from userp2130.oracle.com ([156.151.31.86]) by sfi-mx-4.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) id 1f24T5-006Q6G-B1 for linux-f2fs-devel@lists.sourceforge.net; Sat, 31 Mar 2018 00:34:05 +0000 In-Reply-To: <20180330034245.10462-44-willy@infradead.org> Content-Language: en-US List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net To: Matthew Wilcox , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Cc: linux-nilfs@vger.kernel.org, Jan Kara , Jeff Layton , Matthew Wilcox , James Simmons , Jaegeuk Kim , Andreas Dilger , Nicholas Piggin , linux-f2fs-devel@lists.sourceforge.net, Oleg Drokin , Ryusuke Konishi , Lukas Czerner , Ross Zwisler , Christoph Hellwig , Goldwyn Rodrigues On 03/29/2018 08:42 PM, Matthew Wilcox wrote: > From: Matthew Wilcox > > 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 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 > --- > 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 > > /* > - * 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); > } > > /* > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot