On Wed, Feb 21, 2024 at 10:08:33AM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2024 17:50:44 +0000
> Beau Belgrave <be...@linux.microsoft.com> wrote:
>
> > +static char *user_event_group_system_multi_name(void)
> > +{
> > + char *system_name;
> > + int len = sizeof(USER_EVENTS_MULTI_SYSTEM) + 1;
>
> FYI, the sizeof() will include the "\0" so no need for "+ 1", but I don't
> think this matters as for what I mention below.
>
> > +
> > + system_name = kmalloc(len, GFP_KERNEL);
> > +
> > + if (!system_name)
> > + return NULL;
> > +
> > + snprintf(system_name, len, "%s", USER_EVENTS_MULTI_SYSTEM);
> > +
> > + return system_name;
>
> Hmm, the above looks like an open coded version of:
>
> system_name = kstrdup(USER_EVENTS_MULTI_SYSTEM, GFP_KERNEL);
>
That's much cleaner, I'll move to that.
>
> > +}
> > +
> > static struct user_event_group *current_user_event_group(void)
> > {
> > return init_group;
> > @@ -367,6 +390,11 @@ static struct user_event_group
> > *user_event_group_create(void)
> > if (!group->system_name)
> > goto error;
> >
> > + group->system_multi_name = user_event_group_system_multi_name();
> > +
> > + if (!group->system_multi_name)
> > + goto error;
> > +
> > mutex_init(&group->reg_mutex);
> > hash_init(group->register_table);
> >
> > @@ -1482,6 +1510,11 @@ static int destroy_user_event(struct user_event
> > *user)
> > hash_del(&user->node);
> >
> > user_event_destroy_validators(user);
> > +
> > + /* If we have different names, both must be freed */
> > + if (EVENT_NAME(user) != EVENT_TP_NAME(user))
> > + kfree(EVENT_TP_NAME(user));
> > +
> > kfree(user->call.print_fmt);
> > kfree(EVENT_NAME(user));
> > kfree(user);
> > @@ -1504,12 +1537,24 @@ static struct user_event *find_user_event(struct
> > user_event_group *group,
> > *outkey = key;
> >
> > hash_for_each_possible(group->register_table, user, node, key) {
> > + /*
> > + * Single-format events shouldn't return multi-format
> > + * events. Callers expect the underlying tracepoint to match
> > + * the name exactly in these cases. Only check like-formats.
> > + */
> > + if (EVENT_MULTI_FORMAT(flags) !=
> > EVENT_MULTI_FORMAT(user->reg_flags))
> > + continue;
> > +
> > if (strcmp(EVENT_NAME(user), name))
> > continue;
> >
> > if (user_fields_match(user, argc, argv))
> > return user_event_get(user);
> >
> > + /* Scan others if this is a multi-format event */
> > + if (EVENT_MULTI_FORMAT(flags))
> > + continue;
> > +
> > return ERR_PTR(-EADDRINUSE);
> > }
> >
> > @@ -1889,8 +1934,12 @@ static bool user_event_match(const char *system,
> > const char *event,
> > struct user_event *user = container_of(ev, struct user_event, devent);
> > bool match;
> >
> > - match = strcmp(EVENT_NAME(user), event) == 0 &&
> > - (!system || strcmp(system, USER_EVENTS_SYSTEM) == 0);
> > + match = strcmp(EVENT_NAME(user), event) == 0;
> > +
> > + if (match && system) {
> > + match = strcmp(system, user->group->system_name) == 0 ||
> > + strcmp(system, user->group->system_multi_name) == 0;
> > + }
> >
> > if (match)
> > match = user_fields_match(user, argc, argv);
> > @@ -1923,6 +1972,39 @@ static int user_event_trace_register(struct
> > user_event *user)
> > return ret;
> > }
> >
> > +static int user_event_set_tp_name(struct user_event *user)
> > +{
> > + lockdep_assert_held(&user->group->reg_mutex);
> > +
> > + if (EVENT_MULTI_FORMAT(user->reg_flags)) {
> > + char *multi_name;
> > + int len;
> > +
> > + len = snprintf(NULL, 0, "%s.%llx", user->reg_name,
> > + user->group->multi_id) + 1;
> > +
> > + multi_name = kzalloc(len, GFP_KERNEL_ACCOUNT);
> > +
> > + if (!multi_name)
> > + return -ENOMEM;
> > +
> > + snprintf(multi_name, len, "%s.%llx", user->reg_name,
> > + user->group->multi_id);
>
> I believe the above can be replaced with:
>
> multi_name = kasprintf(GFP_KERNEL_ACCOUNT, "%s.%llx",
> user->reg_name,
> user->group->multi_id);
> if (!multi_name)
> return -ENOMEM;
>
Great, I'll move to that as well and validate.
Thanks,
-Beau
> -- Steve
>
> > +
> > + user->call.name = multi_name;
> > + user->tracepoint.name = multi_name;
> > +
> > + /* Inc to ensure unique multi-event name next time */
> > + user->group->multi_id++;
> > + } else {
> > + /* Non Multi-format uses register name */
> > + user->call.name = user->reg_name;
> > + user->tracepoint.name = user->reg_name;
> > + }
> > +
> > + return 0;
> > +}
