Re: [PATCH drm-next v2 03/16] maple_tree: split up MA_STATE() macro

Danilo Krummrich
2023-02-20T14:39:12Z

On 2/17/23 20:45, Matthew Wilcox wrote:
> On Fri, Feb 17, 2023 at 02:44:09PM +0100, Danilo Krummrich wrote:
>> \#define SAMPLE_ITER(name, __mgr) \
>> struct sample_iter name = { \
>> .mas = __MA_STATE(&(__mgr)->mt, 0, 0),
>
> This is usually called MA_STATE_INIT()

Yep, that's better.

>
>> #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))
>
> This is a bad iterator design. It's usually best to do this:
>
> struct sample *sample;
> SAMPLE_ITERATOR(si, min);
>
> sample_iter_for_each(&si, sample, max) {
> frob(mgr, sample);
> }
>

The reason why I don't set index (and max) within SAMPLE_ITER() is that
the range to iterate might not yet be known at that time, so I thought
it could just be set in sample_iter_for_each_range().

However, I see that this might prevail users to assume that it's safe to
iterate a range based on the same iterator instance multiple times
though. Instead users should maybe move the tree walk to another
function once the range is known.

The reason for the payload structure to be part of the iterator is that
I have two maple trees in the GPUVA manager and hence two different
payload types. Within the iterator structure they're just within a union
allowing me to implement the tree walk macro just once rather than twice.

Anyway, I feel like your approach looks cleaner, hence I'll change it.

> I don't mind splitting apart MA_STATE_INIT from MA_STATE, and if you
> do that, we can also use it in VMA_ITERATOR.
>