Re: [PATCH v2 3/5] tracing/user_events: Add auto cleanup and a flag to persist events

Beau Belgrave
2023-06-08T21:23:08Z

On Thu, Jun 08, 2023 at 04:25:57PM -0400, Steven Rostedt wrote:
> On Mon, 5 Jun 2023 16:38:58 -0700
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
>
> > Currently user events need to be manually deleted via the delete IOCTL
> > call or via the dynamic_events file. Most operators and processes wish
> > to have these events auto cleanup when they are no longer used by
> > anything to prevent them piling without manual maintenance. However,
> > some operators may not want this, such as pre-registering events via the
> > dynamic_events tracefs file.
> >
> > Add a persist flag to user facing header and honor it within the
> > register IOCTL call. Add max flag as well to ensure that only known
> > flags can be used now and in the future. Update user_event_put() to
> > attempt an auto delete of the event if it's the last reference. The
> > auto delete must run in a work queue to ensure proper behavior of
> > class->reg() invocations that don't expect the call to go away from
> > underneath them during the unregister. Add work_struct to user_event
> > struct to ensure we can do this reliably.
> >
> > Link: https://lore.kernel.org/linux-trace-kernel/20230518093600.3f119d68@rorschach.local.home/
>
> Since there still seems to be some controversy over the persistent events,
> could we hold off on implementing them until next merge window? That is, I
> would like to only have the fd owned events for this release, which would
> give us time to hash out any of the issues with persistent events.
>
> If they are not in, then they are not an API, but once they are in, then we
> are stuck with them. I believe everyone is fine with the fd owned events,
> right?
>

I am fine with this approach, however, FD owned events only means that
anyone using the /sys/kernel/tracing/dynamic_events will have the event
deleted immediately.

Should we flat out issue an error instead of having it work, but then
removed immediately?

NOTE:
I'm waiting for the other user_event patches to land in the tracing
for-next since there will be conflicts and I want to make sure I get
good coverage with catching all the put/get refs.

> >
> > Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > ---
> > include/uapi/linux/user_events.h | 10 ++-
> > kernel/trace/trace_events_user.c | 118 +++++++++++++++++++++++++++----
> > 2 files changed, 114 insertions(+), 14 deletions(-)
> >
> >
>
> That is we can keep all the code of this patch, but:
>
> > static __always_inline __must_check
> > @@ -1609,7 +1695,8 @@ static int user_event_create(const char *raw_command)
> >
> > mutex_lock(&group->reg_mutex);
> >
> > - ret = user_event_parse_cmd(group, name, &user, 0);
> > + /* Dyn events persist, otherwise they would cleanup immediately */
> > + ret = user_event_parse_cmd(group, name, &user, USER_EVENT_REG_PERSIST);
> >
> > if (!ret)
> > user_event_put(user, false);
> > @@ -1843,8 +1930,13 @@ static int user_event_parse(struct user_event_group *group, char *name,
> >
>
> Add here:
>
> if (reg_flags) {
> /* Holding off implementing PERSIST events */
> ret = -EINVAL;
> goto put_user_lock;
> }
>
> Which also reminds me. We should return EINVAL if any flags that we do not
> know about is set. Otherwise when we do implement new flags, the user will
> not know if they are taking effect or not.
>

We do that in user_reg_get, but point taken, some flag could come
through dynamic_events interface and then we'd miss it.

Thanks,
-Beau

> -- Steve
>
>
> > user->reg_flags = reg_flags;
> >
> > - /* Ensure we track self ref and caller ref (2) */
> > - refcount_set(&user->refcnt, 2);
> > + if (user->reg_flags & USER_EVENT_REG_PERSIST) {
> > + /* Ensure we track self ref and caller ref (2) */
> > + refcount_set(&user->refcnt, 2);
> > + } else {
> > + /* Ensure we track only caller ref (1) */
> > + refcount_set(&user->refcnt, 1);
> > + }
> >
> > dyn_event_init(&user->devent, &user_event_dops);
> > dyn_event_add(&user->devent, &user->call);
> > @@ -2066,8 +2158,8 @@ static long user_reg_get(struct user_reg __user *ureg, struct user_reg *kreg)
> > if (ret)
> > return ret;
> >
> > - /* Ensure no flags, since we don't support any yet */
> > - if (kreg->flags != 0)
> > + /* Ensure only valid flags */
> > + if (kreg->flags & ~(USER_EVENT_REG_MAX-1))
> > return -EINVAL;
> >
> > /* Ensure supported size */