Re: [PATCH v2 0/2] Lock and Pointer guards

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Jun 06, 2023 at 04:22:26PM -0700, Linus Torvalds wrote:
> On Tue, Jun 6, 2023 at 11:08 AM Peter Zijlstra <peterz@infradead.org> wrote:>
> > Would it all be less offensive if I did: s/guard/cleanup/ on the whole
> > thing?
> 
> It's more than "guard" for me.
> 
> What is "ptr"? Why? We already know of at least one case where it's
> not a pointer at all, ie 'struct fd'.

(so in my view struct fd is nothing more than a fat pointer)

> So I *really* hate the naming. Absolutely none of it makes sense to
> me. One part is a nonsensical name apparently based on a special-case
> operation, and the other part is a nonsensical type from just one
> random - if common - implementation issue.
> 
> What you want to do is to have a way to define and name a
> "constructor/desctructor" pair for an arbitrary type - *not*
> necessarily a pointer - and then optionally a way to say "Oh, don't do
> the destructor, because I'm actually going to use it long-term".

Yes, so when it's a 'pointer', that part becomes assigning it NULL (or
fdnull in the struct fd case). For example:

DEFINE_PTR_CLEANUP(kfree, void *, kfree(_C))

	ptr_cleanup(kfree, mem) = kzalloc(....);
	if (!mem)
		return -ENOMEM;

	object = mem;

	// build object with more failure cases

	mem = NULL;          // object is a success, we keep it.
	return object;

> I said "cleanup", but that's not right either, since we always have to
> have that initializer too.

I've found that for most things the initializer part isn't actually that
important. Consider that struct fd thing again; perf has a helper:

static inline struct fd perf_fget_light(int fd)
{
	struct fd f = fdget(fd);
	if (!f.file)
		return fdnull;

	if (f.file->f_op != &perf_fops) {
		fdput(f);
		return fdnull;
	}
	return f;
}

So now we have both fdget() and perf_fget_light() to obtain a struct fd,
both need fdput().

The pointer with destructor semantics works for both:

DEFINE_PTR_CLEANUP(fdput, struct fd, fdput(_C))

	ptr_cleanup(fdput, f) = perf_fget_light(fd);

or, somewhere else:

	ptr_cleanup(fdput, f) = fdget(fd);


The same is true for kfree(), we have a giant pile of allocation
functions that all are freed with kfree(): kmalloc(), kzalloc(),
kmalloc_node(), kzalloc_node(), krealloc(), kmalloc_array(),
krealloc_array(), kcalloc(), etc..

> Maybe just bite the bullet, and call the damn thing a "class", and
> have some syntax like
> 
>      DEFINE_CLASS(name, type, exit, init, initarg...);
> 
> to create the infrastructure for some named 'class'. So you'd have
> 
>     DEFINE_CLASS(mutex, struct mutex *,
>         mutex_unlock(*_P),
>         ({mutex_lock(mutex); mutex;}), struct mutex *mutex)
> 
> to define the mutex "class", and do
> 
>     DEFINE_CLASS(fd, struct fd,
>         fdput(*_P),
>         fdget(f), int f)
> 
> for the 'struct fd' thing.

Right; that is very close to what I have. And certainly useful --
although as per the above, perhaps not so for the struct fd case.

> Then to _instantiate_ one of those, you'd do
> 
>     INSTANTIATE_CLASS(name, var)
> 
> which would expand to
> 
>     class_name_type var
>         __attribute__((__cleanup__(class_name_destructor))) =
> class_name_constructor
> 
> and the magic of that syntax is that you'd actually use that
> "INSTANTIATE_CLASS()" with the argument to the init function
> afterwards, so you'd actually do
> 
>     INSTANTIATE_CLASS(mutex, n)(&sched_domains_mutex);
> 
> to create a variable 'n' of class 'mutex', where the
> class_mutex_constructor gets the pointer to 'sched_domain_mutex' as
> the argument.

Yes, I had actually considered this syntax, and I really like it. The
only reason I hadn't done that is because the for-loop thing, there I
couldn't make it work.

> I'm sure there's something horribly wrong in the above, but my point
> is that I'd really like this to make naming and conceptual sense.

Right, I hear ya. So the asymmetric case (iow destructor only) could be
seen as using the copy-constructor.

#define DEFINE_CLASS(name, type, exit, init, init_args...)		\
typedef type class_##name##_t;						\
static inline void class_##name##_destructor(type *this)		\
{ type THIS = *this; exit; }						\
static inline type class_##name##_constructor(init_args)		\
{ type THIS = init; return THIS; }

#define __INSTANTIATE_VAR(name, var)					\
	class_##name##_t var __cleanup(class_##name##_destructor)

#define INSTANTIATE_CLASS(name, var)					\
	__INSTANTIATE_VAR(name, var) = class_##name##_constructor


DEFINE_CLASS(fd, struct fd, fdput(THIS), f, struct fd f)

	INSTANTIATE_CLASS(fd, f)(perf_fget_light(fd));


Alternatively, you be OK with exposing INSTANTIATE_VAR() to easily
circumvent the default constructor?

And/or how about EXTEND_CLASS(), something like so?

#define EXTEND_CLASS(name, ext, init, init_args...)			\
typedef class_##name##_t class_##name##ext##_t;				\
static inline void class_##name##ext##_destructor(class_##name##_t *this) \
{ class_##name##_destructor(this); }					\
static inline type class_##name##ext##_constructor(init_args)		\
{ type THIS = init; return THIS; }


DEFINE_CLASS(fd, struct fd, fdput(THIS), fdget(fd), int fd)
EXTEND_CLASS(fd, _perf, perf_fget_light(fd), int fd)

	INSTANTIATE_CLASS(fd_perf, f)(fd);


> And at THAT point, you can do this:
> 
>     #define mutex_guard(m) \
>         INSTANTIATE_CLASS(mutex, __UNIQUE_ID(mutex))(m)
> 
> and now you can do
> 
>        mutex_guard(&sched_domains_mutex);

So the 'problem' is the amount of different guards I ended up having and
you can't have macro's define more macros :/

Which is how I ended up with the:

	guard(mutex, &sched_domains_mutex);

syntax.

This can ofcourse be achieved using the above CLASS thing like:

DEFINE_CLASS(mutex, struct mutex *, mutex_unlock(THIS),
	     ({ mutex_lock(m); m; }), struct mutex *m)

#define named_guard(name, var, args...)					\
	INSTANTIATE_CLASS(name, var)(args)

#define guard(name, args...)						\
	named_guard(name, __UNIQUE_ID(guard), args)

#define scoped_guard(name, args...)					\
	for (named_guard(name, scope, args),				\
	     *done = NULL; !done; done = (void *)1)


With the understanding they're only to be used for locks.

Also, I'm already tired of writing INSTANTIATE.. would:

	CLASS(fd, f)(fd);

	VAR(kfree, mem) = kzalloc_node(...);

be acceptable shorthand?