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);
> +}
> +
> 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;
-- 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;
> +}
