Re: [Intel-xe] [PATCH v2 13/31] maple_tree: split up MA_STATE() macro

[Matthew Brost <matthew.brost@intel.com>]

On Tue, May 09, 2023 at 03:21:56PM +0200, Thomas Hellström wrote:
> 
> On 5/2/23 02:17, Matthew Brost wrote:
> > From: Danilo Krummrich <dakr@redhat.com>
> > 
> > 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.
> 
> I might misunderstand the patch, but isn't the real purpose to provide an
> MA_STATE initializer,and the way to achieve that is to split up the MA_STATE
> macro?
> 

This patch isn't mine and on dri-devel. Can you put your comments there?
https://patchwork.freedesktop.org/patch/530673/?series=112994&rev=4

Matt

> > 
> > Example:
> > 
> > struct sample_iterator {
> >          struct ma_state mas;
> >          struct sample_mgr *mgr;
> > };
> > 
> > \#define SAMPLE_ITERATOR(name, __mgr, start)                    \
> >          struct sample_iterator name = {                         \
> >                  .mas = MA_STATE_INIT(&(__mgr)->mt, start, 0),   \
> >                  .mgr = __mgr,                                   \
> >          }
> > 
> > \#define sample_iter_for_each_range(it__, entry__, end__) \
> >          mas_for_each(&(it__).mas, entry__, end__)
> > 
> > --
> > 
> > struct sample *sample;
> > SAMPLE_ITERATOR(si, min);
> > 
> > sample_iter_for_each_range(&si, sample, max) {
> >          frob(mgr, sample);
> > }
> > 
> > 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 1fadb5f5978b..87d55334f1c2 100644
> > --- a/include/linux/maple_tree.h
> > +++ b/include/linux/maple_tree.h
> > @@ -423,8 +423,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_INIT(mt, first, end)					\
> > +	{								\
> 
> Naming: following the convention in, for example, the mutex and ww mutex
> code this should've been called
> 
> __MA_STATE_INITIALIZER(),
> 
> whereas the decapitalized name ma_state_init() would've been a (possibly
> inline) init function if it existed.
> 
> But this all should ofc be run by the maple tree maintainer(s).
> 
> FWIW, with these things addressed the change LGTM.
> 
> /Thomas
> 
> 
> >   		.tree = mt,						\
> >   		.index = first,						\
> >   		.last = end,						\
> > @@ -435,6 +435,9 @@ struct ma_wr_state {
> >   		.mas_flags = 0,						\
> >   	}
> > +#define MA_STATE(name, mt, first, end)					\
> > +	struct ma_state name = MA_STATE_INIT(mt, first, end)
> > +
> >   #define MA_WR_STATE(name, ma_state, wr_entry)				\
> >   	struct ma_wr_state name = {					\
> >   		.mas = ma_state,					\