Re: [PATCH v2 1/2] locking: Introduce __cleanup__ based guards

[Linus Torvalds]

On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>> > +                                                                             \
> > +static inline void ptr_guard_##_type##_cleanup(_Type **_ptr)                 \
> > +{                                                                            \
> > +     _Type *_G = *_ptr;                                                      \
> > +     if (_G)                                                                 \
> > +             _Put(_G);                                                       \
> > +}
> > +
> > +#define ptr_guard(_type, _name)                                                      \
> > +     ptr_guard_##_type##_t _name __cleanup(ptr_guard_##_type##_cleanup)
> > +
>
> Ha, and then I wanted to create an fdput() guard... :/

I have to say, I'm not super-convinced about the macros to create
these guard functions and types.

I suspect that it would be better to literally just declare the guard
types directly.  For the fdget idiom, you really just want the guard
type to *be* the 'struct fd'.

And I think you made those macros just make too many assumptions.

It's not just "struct fd", for example.  If you want to do things like
wrap 'local_irq_save', again it's not a pointer, the context is just
'unsigned long irqflags'.

And I really don't think those type-creation macros buy you anything.

What's wrong with just writing it out:

    typedef struct fd guard_fdget_type_t;
    static inline struct fd guard_fdget_init(int fd)
    { return fdget(fd); }
    static inline void guard_fdget_exit(struct fd fd)
    { fdput(fd); }

and

    typedef struct mutex *guard_mutex_type_t;
    static inline struct mutex *guard_mutex_init(struct mutex *mutex)
    { mutex_lock(mutex); return mutex; }
    static inline void guard_mutex_exit(struct mutex *mutex)
    { mutex_unlock(mutex); }

I don't think the latter is in the *least* less readable than doing

    DEFINE_LOCK_GUARD_1(mutex, struct mutex,
                mutex_lock(_G->lock),
                mutex_unlock(_G->lock))

which is this magic macro that creates those, and makes them
completely ungreppable - and makes the type system very inflexible.

So I really think that it would be a lot easier to write out the
wrappers explicitly for the few types that really want this.

I dunno.

Once again, I have written example code in my MUA that I have not
tested AT ALL, and that may be fundamentally broken for some very
obvious reason, and I'm just too stupid to see it.

             Linus