Messages in this thread | | | Date | Mon, 20 Feb 2023 14:48:34 +0100 | Subject | Re: [PATCH drm-next v2 03/16] maple_tree: split up MA_STATE() macro | From | Danilo Krummrich <> |
| |
On 2/17/23 19:34, Liam R. Howlett wrote: > * Danilo Krummrich <dakr@redhat.com> [230217 08:44]: >> Split up the MA_STATE() macro such that components using the maple tree >> can easily inherit from struct ma_state and build custom tree walk >> macros to hide their internals from users. >> >> Example: >> >> struct sample_iter { >> struct ma_state mas; >> struct sample_mgr *mgr; >> struct sample_entry *entry; >> }; >> >> \#define SAMPLE_ITER(name, __mgr) \ >> struct sample_iter name = { \ >> .mas = __MA_STATE(&(__mgr)->mt, 0, 0), >> .mgr = __mgr, >> .entry = NULL, >> } > > I see this patch is to allow for anonymous maple states, this looks > good. > > I've a lengthy comment about the iterator that I'm adding here to head > off anyone that may copy your example below. > >> >> \#define sample_iter_for_each_range(it__, start__, end__) \ >> for ((it__).mas.index = start__, (it__).entry = mas_find(&(it__).mas, end__ - 1); \ >> (it__).entry; (it__).entry = mas_find(&(it__).mas, end__ - 1)) > > I see you've added something like the above in your patch set as well. > I'd like to point out that the index isn't the only state information > that needs to be altered here, and in fact, this could go very wrong. > > The maple state has a node and an offset within that node. If you set > the index to lower than the current position of your iterator and call > mas_find() then what happens is somewhat undefined. I expect you will > get the wrong value (most likely either the current value or the very > next one that the iterator is already pointing to). I believe you have > been using a fresh maple state for each iterator in your patches, but I > haven't had a deep look into your code yet.
Yes, I'm aware that I'd need to reset the whole iterator in order to re-use it.
Regarding the other considerations of the iterator design please see my answer to Matthew.
> > We have methods of resetting the iterator and set the range (mas_set() > and mas_set_range()) which are safe for what you are doing, but they > will start the walk from the root node to the index again. > > So, if you know what you are doing is safe, then the way you have > written it will work, but it's worth mentioning that this could occur. > > It is also worth pointing out that it would be much safer to use a > function to do the above so you get type safety.. and I was asked to add > this to the VMA interface by Linus [1], which is on its way upstream [2]. > > 1. https://lore.kernel.org/linux-mm/CAHk-=wg9WQXBGkNdKD2bqocnN73rDswuWsavBB7T-tekykEn_A@mail.gmail.com/ > 2. https://lore.kernel.org/linux-mm/20230120162650.984577-1-Liam.Howlett@oracle.com/
You mean having wrappers like sample_find() instead of directly using mas_find()?
> >> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >> --- >> include/linux/maple_tree.h | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h >> index e594db58a0f1..ca04c900e51a 100644 >> --- a/include/linux/maple_tree.h >> +++ b/include/linux/maple_tree.h >> @@ -424,8 +424,8 @@ struct ma_wr_state { >> #define MA_ERROR(err) \ >> ((struct maple_enode *)(((unsigned long)err << 2) | 2UL)) >> >> -#define MA_STATE(name, mt, first, end) \ >> - struct ma_state name = { \ >> +#define __MA_STATE(mt, first, end) \ >> + { \ >> .tree = mt, \ >> .index = first, \ >> .last = end, \ >> @@ -435,6 +435,9 @@ struct ma_wr_state { >> .alloc = NULL, \ >> } >> >> +#define MA_STATE(name, mt, first, end) \ >> + struct ma_state name = __MA_STATE(mt, first, end) >> + >> #define MA_WR_STATE(name, ma_state, wr_entry) \ >> struct ma_wr_state name = { \ >> .mas = ma_state, \ >> -- >> 2.39.1 >> >
| |