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 -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR