| Message ID | 20200204142514.15826-3-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Speedup page cache truncation | expand |
On Tue, Feb 04, 2020 at 03:25:08PM +0100, Jan Kara wrote: > Currently xas_store() clears marks when stored value is NULL. This is > somewhat counter-intuitive and also causes measurable performance impact > when mark clearing is not needed (e.g. because marks are already clear). > So provide xas_erase() helper (similarly to existing xa_erase()) which > stores NULL at given index and also takes care of clearing marks. Use > this helper from __xa_erase() and item_kill_tree() in tools/testing. In > the following patches, callers that use the mark-clearing property of > xas_store() will be converted to xas_erase() and remaining users can > enjoy better performance. I (finally!) figured out what I don't like about this series. You're changing the semantics of xas_store() without changing the name, so if we have any new users in flight they'll use the new semantics when they're expecting the old. Further, while you've split the patches nicely for review, they're not good for bisection because the semantic change comes right at the end of the series, so any problem due to this series is going to bisect to the end, and not tell us anything useful. What I think this series should do instead is: Patch 1: +#define xas_store(xas, entry) xas_erase(xas, entry) -void *xas_store(struct xa_state *xas, void *entry) +void *__xas_store(struct xa_state *xas, void *entry) - if (!entry) - xas_init_marks(xas); +void *xas_erase(struct xa_state *xas, void *entry) +{ + xas_init_marks(xas); + return __xas_store(xas, entry); +} (also documentation changes) Patches 2-n: Change each user of xas_store() to either xas_erase() or __xas_store() Patch n+1: -#define xas_store(xas, entry) xas_erase(xas, entry) Does that make sense? I'll code it up next week unless you want to.
On Sat 14-03-20 12:54:53, Matthew Wilcox wrote: > On Tue, Feb 04, 2020 at 03:25:08PM +0100, Jan Kara wrote: > > Currently xas_store() clears marks when stored value is NULL. This is > > somewhat counter-intuitive and also causes measurable performance impact > > when mark clearing is not needed (e.g. because marks are already clear). > > So provide xas_erase() helper (similarly to existing xa_erase()) which > > stores NULL at given index and also takes care of clearing marks. Use > > this helper from __xa_erase() and item_kill_tree() in tools/testing. In > > the following patches, callers that use the mark-clearing property of > > xas_store() will be converted to xas_erase() and remaining users can > > enjoy better performance. > > I (finally!) figured out what I don't like about this series. You're > changing the semantics of xas_store() without changing the name, so > if we have any new users in flight they'll use the new semantics when > they're expecting the old. > > Further, while you've split the patches nicely for review, they're not > good for bisection because the semantic change comes right at the end > of the series, so any problem due to this series is going to bisect to > the end, and not tell us anything useful. OK, fair enough. > What I think this series should do instead is: > > Patch 1: > +#define xas_store(xas, entry) xas_erase(xas, entry) > -void *xas_store(struct xa_state *xas, void *entry) > +void *__xas_store(struct xa_state *xas, void *entry) > - if (!entry) > - xas_init_marks(xas); > +void *xas_erase(struct xa_state *xas, void *entry) > +{ > + xas_init_marks(xas); > + return __xas_store(xas, entry); > +} > (also documentation changes) > > Patches 2-n: > Change each user of xas_store() to either xas_erase() or __xas_store() > > Patch n+1: > -#define xas_store(xas, entry) xas_erase(xas, entry) > > Does that make sense? I'll code it up next week unless you want to. Fine by me and I agree the change is going to be more robust this way. I just slightly dislike that you end up with __xas_store() with two underscores which won't have a good meaning after the series is done but I can certainly live with that :). If you've time to reorganize the series this week, then go ahead. I've just returned from a week of vacation so it will take a while for me to catch up... Thanks for having a look! Honza
On Tue, Feb 04, 2020 at 03:25:08PM +0100, Jan Kara wrote: > +void *xas_erase(struct xa_state *xas) > +{ > + void *entry; > + > + entry = xas_store(xas, NULL); > + xas_init_marks(xas); > + > + return entry; > +} > +EXPORT_SYMBOL(xas_erase); I didn't have a test case to show this, but ... +static noinline void check_multi_store_4(struct xarray *xa) +{ + XA_BUG_ON(xa, xa_marked(xa, XA_MARK_0)); + XA_BUG_ON(xa, !xa_empty(xa)); + + xa_store_index(xa, 0, GFP_KERNEL); + xa_store_index(xa, 2, GFP_KERNEL); + xa_set_mark(xa, 0, XA_MARK_0); + xa_set_mark(xa, 2, XA_MARK_0); + + xa_store_order(xa, 0, 2, NULL, GFP_KERNEL); + XA_BUG_ON(xa, xa_marked(xa, XA_MARK_0)); + xa_destroy(xa); +} shows a problem. Because we delete all the entries in the tree, xas_delete_node() sets the xas->xa_node to XAS_BOUNDS. This fixes it: @@ -492,7 +492,6 @@ static void xas_delete_node(struct xa_state *xas) if (!parent) { xas->xa->xa_head = NULL; - xas->xa_node = XAS_BOUNDS; return; } (it leaves xas->xa_node set to NULL, which makes the above work correctly because NULL is used to mean the one element at index 0 of the array) Now I'm wondering if it's going to break anything, though. The test suite runs successfully, but it can't be exhaustive.
On Tue 17-03-20 08:28:50, Matthew Wilcox wrote: > On Tue, Feb 04, 2020 at 03:25:08PM +0100, Jan Kara wrote: > > +void *xas_erase(struct xa_state *xas) > > +{ > > + void *entry; > > + > > + entry = xas_store(xas, NULL); > > + xas_init_marks(xas); > > + > > + return entry; > > +} > > +EXPORT_SYMBOL(xas_erase); > > I didn't have a test case to show this, but ... > > +static noinline void check_multi_store_4(struct xarray *xa) > +{ > + XA_BUG_ON(xa, xa_marked(xa, XA_MARK_0)); > + XA_BUG_ON(xa, !xa_empty(xa)); > + > + xa_store_index(xa, 0, GFP_KERNEL); > + xa_store_index(xa, 2, GFP_KERNEL); > + xa_set_mark(xa, 0, XA_MARK_0); > + xa_set_mark(xa, 2, XA_MARK_0); > + > + xa_store_order(xa, 0, 2, NULL, GFP_KERNEL); > + XA_BUG_ON(xa, xa_marked(xa, XA_MARK_0)); > + xa_destroy(xa); > +} > > shows a problem. Because we delete all the entries in the tree, > xas_delete_node() sets the xas->xa_node to XAS_BOUNDS. This fixes it: > > @@ -492,7 +492,6 @@ static void xas_delete_node(struct xa_state *xas) > > if (!parent) { > xas->xa->xa_head = NULL; > - xas->xa_node = XAS_BOUNDS; > return; > } > > (it leaves xas->xa_node set to NULL, which makes the above work correctly > because NULL is used to mean the one element at index 0 of the array) > > Now I'm wondering if it's going to break anything, though. The test suite > runs successfully, but it can't be exhaustive. I finally got back to this. Sorry for the delay. I was pondering about the change you suggested for xas_delete_node() and I don't quite like it - for the example you've described, it would be kind of the right thing. But if we'd have an example like: + xa_store_index(xa, 4, GFP_KERNEL); + xa_set_mark(xa, 4, XA_MARK_0); + + xa_store(xa, 4, NULL, GFP_KERNEL); then having XAS_BOUNDS set after the store is IMO what should happen because index 4 stored in xa_index cannot be stored in the array as it currently is. So leaving xa_node set to NULL looks confusing. So I think what I'll do is move xas_init_marks() in xas_erase() before xas_store(). I'll also need to add xas_load() there because that isn't guaranteed to have happened on the xas yet and xas_init_marks() expects xas_load() has already ran... Honza
diff --git a/include/linux/xarray.h b/include/linux/xarray.h index 5370716d7010..be6c6950837e 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -1491,6 +1491,7 @@ static inline bool xas_retry(struct xa_state *xas, const void *entry) void *xas_load(struct xa_state *); void *xas_store(struct xa_state *, void *entry); +void *xas_erase(struct xa_state *); void *xas_find(struct xa_state *, unsigned long max); void *xas_find_conflict(struct xa_state *); diff --git a/lib/xarray.c b/lib/xarray.c index 1d9fab7db8da..ae8b7070e82c 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1319,6 +1319,28 @@ static void *xas_result(struct xa_state *xas, void *curr) return curr; } +/** + * xas_erase() - Erase this entry from the XArray + * @xas: XArray operation state. + * + * After this function returns, loading from @index will return %NULL. The + * function also clears all marks associated with the @index. If the index is + * part of a multi-index entry, all indices will be erased and none of the + * entries will be part of a multi-index entry. + * + * Return: The entry which used to be at this index. + */ +void *xas_erase(struct xa_state *xas) +{ + void *entry; + + entry = xas_store(xas, NULL); + xas_init_marks(xas); + + return entry; +} +EXPORT_SYMBOL(xas_erase); + /** * __xa_erase() - Erase this entry from the XArray while locked. * @xa: XArray. @@ -1334,7 +1356,7 @@ static void *xas_result(struct xa_state *xas, void *curr) void *__xa_erase(struct xarray *xa, unsigned long index) { XA_STATE(xas, xa, index); - return xas_result(&xas, xas_store(&xas, NULL)); + return xas_result(&xas, xas_erase(&xas)); } EXPORT_SYMBOL(__xa_erase); diff --git a/tools/testing/radix-tree/test.c b/tools/testing/radix-tree/test.c index a15d0512e633..07dc2b4dc587 100644 --- a/tools/testing/radix-tree/test.c +++ b/tools/testing/radix-tree/test.c @@ -261,7 +261,7 @@ void item_kill_tree(struct xarray *xa) if (!xa_is_value(entry)) { item_free(entry, xas.xa_index); } - xas_store(&xas, NULL); + xas_erase(&xas); } assert(xa_empty(xa));
Currently xas_store() clears marks when stored value is NULL. This is somewhat counter-intuitive and also causes measurable performance impact when mark clearing is not needed (e.g. because marks are already clear). So provide xas_erase() helper (similarly to existing xa_erase()) which stores NULL at given index and also takes care of clearing marks. Use this helper from __xa_erase() and item_kill_tree() in tools/testing. In the following patches, callers that use the mark-clearing property of xas_store() will be converted to xas_erase() and remaining users can enjoy better performance. Signed-off-by: Jan Kara <jack@suse.cz> --- include/linux/xarray.h | 1 + lib/xarray.c | 24 +++++++++++++++++++++++- tools/testing/radix-tree/test.c | 2 +- 3 files changed, 25 insertions(+), 2 deletions(-)