Re: Truncate regression due to commit 69b6c1319b6

[Matthew Wilcox <willy@infradead.org>]

On Tue, Feb 26, 2019 at 05:56:28PM +0100, Jan Kara wrote:
> after some peripeties, I was able to bisect down to a regression in
> truncate performance caused by commit 69b6c1319b6 "mm: Convert truncate to
> XArray".

[...]

> I've gathered also perf profiles but from the first look they don't show
> anything surprising besides xas_load() and xas_store() taking up more time
> than original counterparts did. I'll try to dig more into this but any idea
> is appreciated.

Well, that's a short and sweet little commit.  Stripped of comment
changes, it's just:

-       struct radix_tree_node *node;
-       void **slot;
+       XA_STATE(xas, &mapping->i_pages, index);
 
-       if (!__radix_tree_lookup(&mapping->i_pages, index, &node, &slot))
+       xas_set_update(&xas, workingset_update_node);
+       if (xas_load(&xas) != entry)
                return;
-       if (*slot != entry)
-               return;
-       __radix_tree_replace(&mapping->i_pages, node, slot, NULL,
-                            workingset_update_node);
+       xas_store(&xas, NULL);

I have a few reactions to this:

1. I'm concerned that the XArray may generally be slower than the radix
tree was.  I didn't notice that in my testing, but maybe I didn't do
the right tests.

2. The setup overhead of the XA_STATE might be a problem.
If so, we can do some batching in order to improve things.
I suspect your test is calling __clear_shadow_entry through the
truncate_exceptional_pvec_entries() path, which is already a batch.
Maybe something like patch [1] at the end of this mail.

3. Perhaps we can actually get rid of truncate_exceptional_pvec_entries().
It seems a little daft for page_cache_delete_batch() to skip value
entries, only for truncate_exceptional_pvec_entries() to erase them in
a second pass.  Truncation is truncation, and perhaps we can handle all
of it in one place?

4. Now that calling through a function pointer is expensive, thanks to
Spectre/Meltdown/..., I've been considering removing the general-purpose
update function, which is only used by the page cache.  Instead move parts
of workingset.c into the XArray code and use a bit in the xa_flags to
indicate that the node should be tracked on an LRU if it contains only
value entries.

[1]

diff --git a/mm/truncate.c b/mm/truncate.c
index 798e7ccfb030..9384f48eff2a 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -31,23 +31,23 @@
  * lock.
  */
 static inline void __clear_shadow_entry(struct address_space *mapping,
-				pgoff_t index, void *entry)
+		struct xa_state *xas, void *entry)
 {
-	XA_STATE(xas, &mapping->i_pages, index);
-
-	xas_set_update(&xas, workingset_update_node);
-	if (xas_load(&xas) != entry)
+	if (xas_load(xas) != entry)
 		return;
-	xas_store(&xas, NULL);
+	xas_store(xas, NULL);
 	mapping->nrexceptional--;
 }
 
 static void clear_shadow_entry(struct address_space *mapping, pgoff_t index,
 			       void *entry)
 {
-	xa_lock_irq(&mapping->i_pages);
-	__clear_shadow_entry(mapping, index, entry);
-	xa_unlock_irq(&mapping->i_pages);
+	XA_STATE(xas, &mapping->i_pages, index);
+	xas_set_update(&xas, workingset_update_node);
+
+	xas_lock_irq(&xas);
+	__clear_shadow_entry(mapping, &xas, entry);
+	xas_unlock_irq(&xas);
 }
 
 /*
@@ -59,9 +59,12 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
 				struct pagevec *pvec, pgoff_t *indices,
 				pgoff_t end)
 {
+	XA_STATE(xas, &mapping->i_pages, 0);
 	int i, j;
 	bool dax, lock;
 
+	xas_set_update(&xas, workingset_update_node);
+
 	/* Handled by shmem itself */
 	if (shmem_mapping(mapping))
 		return;
@@ -95,7 +98,8 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
 			continue;
 		}
 
-		__clear_shadow_entry(mapping, index, page);
+		xas_set(&xas, index);
+		__clear_shadow_entry(mapping, &xas, page);
 	}
 
 	if (lock)