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@xxxxxxxxxxxxx> 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?
