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

Linus Torvalds
2023-05-26T21:56:13Z

On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <peterz@infradead.org> 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