| Message ID | 20200204142514.15826-4-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:09PM +0100, Jan Kara wrote: > __xa_cmpxchg() relies on xas_store() to set XA_FREE_MARK when storing > NULL into xarray that has free tracking enabled. Make the setting of > XA_FREE_MARK explicit similarly as its clearing currently it. > > Signed-off-by: Jan Kara <jack@suse.cz> > lib/xarray.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/lib/xarray.c b/lib/xarray.c > index ae8b7070e82c..4e32497c51bd 100644 > +++ b/lib/xarray.c > @@ -1477,8 +1477,12 @@ void *__xa_cmpxchg(struct xarray *xa, unsigned long index, > curr = xas_load(&xas); > if (curr == old) { > xas_store(&xas, entry); > - if (xa_track_free(xa) && entry && !curr) > - xas_clear_mark(&xas, XA_FREE_MARK); > + if (xa_track_free(xa)) { > + if (entry && !curr) > + xas_clear_mark(&xas, XA_FREE_MARK); > + else if (!entry && curr) > + xas_set_mark(&xas, XA_FREE_MARK); > + } This feels like an optimization that should also happen for __xa_store, which has very similar code: curr = xas_store(&xas, entry); if (xa_track_free(xa)) xas_clear_mark(&xas, XA_FREE_MARK); Something like if (xa_track_free(xa) && entry && !curr) xas_clear_mark(&xas, XA_FREE_MARK); ? Regards, Jason
On Wed 05-02-20 14:45:12, Jason Gunthorpe wrote: > On Tue, Feb 04, 2020 at 03:25:09PM +0100, Jan Kara wrote: > > __xa_cmpxchg() relies on xas_store() to set XA_FREE_MARK when storing > > NULL into xarray that has free tracking enabled. Make the setting of > > XA_FREE_MARK explicit similarly as its clearing currently it. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > lib/xarray.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/lib/xarray.c b/lib/xarray.c > > index ae8b7070e82c..4e32497c51bd 100644 > > +++ b/lib/xarray.c > > @@ -1477,8 +1477,12 @@ void *__xa_cmpxchg(struct xarray *xa, unsigned long index, > > curr = xas_load(&xas); > > if (curr == old) { > > xas_store(&xas, entry); > > - if (xa_track_free(xa) && entry && !curr) > > - xas_clear_mark(&xas, XA_FREE_MARK); > > + if (xa_track_free(xa)) { > > + if (entry && !curr) > > + xas_clear_mark(&xas, XA_FREE_MARK); > > + else if (!entry && curr) > > + xas_set_mark(&xas, XA_FREE_MARK); > > + } > > This feels like an optimization that should also happen for > __xa_store, which has very similar code: > > curr = xas_store(&xas, entry); > if (xa_track_free(xa)) > xas_clear_mark(&xas, XA_FREE_MARK); > > Something like > > if (xa_track_free(xa) && entry && !curr) > xas_clear_mark(&xas, XA_FREE_MARK); > > ? Yeah, entry != NULL is guaranteed for __xa_store() (see how it transforms NULL to XA_ZERO_ENTRY a few lines above) but !curr is probably a good condition to add to save some unnecessary clearing when overwriting existing values. It is unrelated to this patch though, just a separate optimization so I'll add that as a separate patch to the series. Thanks for the idea. Honza
On Tue, Feb 04, 2020 at 03:25:09PM +0100, Jan Kara wrote: > __xa_cmpxchg() relies on xas_store() to set XA_FREE_MARK when storing > NULL into xarray that has free tracking enabled. Make the setting of > XA_FREE_MARK explicit similarly as its clearing currently it. > if (curr == old) { > xas_store(&xas, entry); > - if (xa_track_free(xa) && entry && !curr) > - xas_clear_mark(&xas, XA_FREE_MARK); > + if (xa_track_free(xa)) { > + if (entry && !curr) > + xas_clear_mark(&xas, XA_FREE_MARK); > + else if (!entry && curr) > + xas_set_mark(&xas, XA_FREE_MARK); > + } This isn't right because the entry might have a different mark set on it that would have been cleared before, but now won't be. I should add a test case for that ...
diff --git a/lib/xarray.c b/lib/xarray.c index ae8b7070e82c..4e32497c51bd 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1477,8 +1477,12 @@ void *__xa_cmpxchg(struct xarray *xa, unsigned long index, curr = xas_load(&xas); if (curr == old) { xas_store(&xas, entry); - if (xa_track_free(xa) && entry && !curr) - xas_clear_mark(&xas, XA_FREE_MARK); + if (xa_track_free(xa)) { + if (entry && !curr) + xas_clear_mark(&xas, XA_FREE_MARK); + else if (!entry && curr) + xas_set_mark(&xas, XA_FREE_MARK); + } } } while (__xas_nomem(&xas, gfp));
__xa_cmpxchg() relies on xas_store() to set XA_FREE_MARK when storing NULL into xarray that has free tracking enabled. Make the setting of XA_FREE_MARK explicit similarly as its clearing currently it. Signed-off-by: Jan Kara <jack@suse.cz> --- lib/xarray.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)