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)
