On Tue, Feb 12, 2019 at 05:51:29AM -0800, Matthew Wilcox wrote: >On Tue, Feb 12, 2019 at 06:29:58PM +1100, Tobin C. Harding wrote: >> I had my first go using the XArray today and during that I wondered if >> it was safe to remove items during iteration. Conceptually it seems >> fine and it seemed to work just fine in code - is this something people >> should not be doing for any reason? Is this the best way to traverse >> the tree and get every thing just to erase it? Are we even supposed to >> be thinking this is a tree or should we just be thinking it is an array? > >You should be thinking it's an array. I've done everything I can to >hide the fact that it's implemented as a tree because it's conceptually >an array. > >The xa_for_each() iterator is designed to be extremely robust, at the >expense of some performance. The only state it keeps is the @index, >so you can do anything you like to the XArray during the iteration. > >It's definitely worth being clearer in the documentation, for >the benefit of people who're wondering what the equivalent of >list_for_each_entry_safe() is. So I'll apply this patch in a day or >two unless anybody has further comment on it. > >> (As you might have guessed I _still_ don't know exactly how a radix tree >> works :) > >That is _fine_. As you know I hope to get rid of the radix tree soon ;-) > You mean replace radix tree in whole kernel? That would be a big effort. BTW, have we compared the performance difference? >> Oh, and FTR the XArray is hot - good effort man. >> >> thanks, >> Tobin. >> >> >> Documentation/core-api/xarray.rst | 3 ++- >> include/linux/xarray.h | 2 ++ >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/core-api/xarray.rst b/Documentation/core-api/xarray.rst >> index 5d54b27c6eba..2578e0bdaa17 100644 >> --- a/Documentation/core-api/xarray.rst >> +++ b/Documentation/core-api/xarray.rst >> @@ -97,7 +97,8 @@ You can copy entries out of the XArray into a plain array by calling >> :c:func:`xa_extract`. Or you can iterate over the present entries in >> the XArray by calling :c:func:`xa_for_each`. You may prefer to use >> :c:func:`xa_find` or :c:func:`xa_find_after` to move to the next present >> -entry in the XArray. >> +entry in the XArray. It is safe to call :c:func:`xa_release` on entries >> +as you iterate over the array using :c:func:`xa_for_each`. > >that's spelled `xa_erase` ;-) > >> Calling :c:func:`xa_store_range` stores the same entry in a range >> of indices. If you do this, some of the other operations will behave >> diff --git a/include/linux/xarray.h b/include/linux/xarray.h >> index 5d9d318bcf7a..1f8974281a0a 100644 >> --- a/include/linux/xarray.h >> +++ b/include/linux/xarray.h >> @@ -407,6 +407,8 @@ static inline bool xa_marked(const struct xarray *xa, xa_mark_t mark) >> * you should use the xas_for_each() iterator instead. The xas_for_each() >> * iterator will expand into more inline code than xa_for_each(). >> * >> + * It is safe to erase entries from the XArray as you iterate over it. >> + * >> * Context: Any context. Takes and releases the RCU lock. >> */ >> #define xa_for_each(xa, index, entry) \ >> -- >> 2.20.1 >> -- Wei Yang Help you, Help me
