Re: [PATCH v4 08/10] libtracefs: Use the internal dynamic events API when creating synthetic events

[Yordan Karadzhov <y.karadz@gmail.com>]

On 4.11.21 г. 13:10, Tzvetomir Stoyanov (VMware) wrote:
> Synthetic events are type of ftrace dynamic events. The tracefs library
> has dedicated APIs to manage dynamic events of all types. In order the
> code to be consistent, the creation of synthetic events inside the
> library is reimplemented with these new dynamic events APIs.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>   src/tracefs-hist.c | 103 +++++++++++++++++++++------------------------
>   1 file changed, 47 insertions(+), 56 deletions(-)
> 
> diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
> index 9009dba..08bb2da 100644
> --- a/src/tracefs-hist.c
> +++ b/src/tracefs-hist.c
> @@ -661,6 +661,7 @@ struct tracefs_synth {
>   	struct tep_event	*end_event;
>   	struct action		*actions;
>   	struct action		**next_action;
> +	struct tracefs_dynevent	*dyn_event;
>   	char			*name;
>   	char			**synthetic_fields;
>   	char			**synthetic_args;
> @@ -719,6 +720,7 @@ void tracefs_synth_free(struct tracefs_synth *synth)
>   		synth->actions = action->next;
>   		action_free(action);
>   	}
> +	tracefs_dynevent_free(synth->dyn_event);
>   
>   	free(synth);
>   }
> @@ -889,6 +891,28 @@ synth_init_from(struct tep_handle *tep, const char *start_system,
>   	return synth;
>   }
>   
> +static int alloc_synthetic_event(struct tracefs_synth *synth)
> +{
> +	char *synthetic_format;
> +	const char *field;
> +	int i;
> +
> +	synthetic_format = strdup("");
> +	if (!synthetic_format)
> +		return -1;
> +
> +	for (i = 0; synth->synthetic_fields && synth->synthetic_fields[i]; i++) {
> +		field = synth->synthetic_fields[i];
> +		synthetic_format = append_string(synthetic_format, " ", field);
> +	}
> +
> +	synth->dyn_event = dynevent_alloc(TRACEFS_DYNEVENT_SYNTH, NULL,
> +					  synth->name, NULL, synthetic_format);
> +	free(synthetic_format);
> +
> +	return synth->dyn_event ? 0 : -1;
> +}
> +
>   /**
>    * tracefs_synth_alloc - create a new tracefs_synth instance
>    * @tep: The tep handle that holds the events to work on
> @@ -1609,38 +1633,6 @@ int tracefs_synth_save(struct tracefs_synth *synth,
>   	return 0;
>   }
>   
> -static char *create_synthetic_event(struct tracefs_synth *synth)
> -{
> -	char *synthetic_event;
> -	const char *field;
> -	int i;
> -
> -	synthetic_event = strdup(synth->name);
> -	if (!synthetic_event)
> -		return NULL;
> -
> -	for (i = 0; synth->synthetic_fields && synth->synthetic_fields[i]; i++) {
> -		field = synth->synthetic_fields[i];
> -		synthetic_event = append_string(synthetic_event, " ", field);
> -	}
> -
> -	return synthetic_event;
> -}
> -
> -static int remove_synthetic(const char *synthetic)
> -{
> -	char *str;
> -	int ret;
> -
> -	ret = asprintf(&str, "!%s", synthetic);
> -	if (ret < 0)
> -		return -1;
> -
> -	ret = tracefs_instance_file_append(NULL, "synthetic_events", str);
> -	free(str);
> -	return ret < 0 ? -1 : 0;
> -}
> -
>   static int remove_hist(struct tracefs_instance *instance,
>   		       struct tep_event *event, const char *hist)
>   {
> @@ -1919,7 +1911,6 @@ tracefs_synth_get_start_hist(struct tracefs_synth *synth)
>   int tracefs_synth_create(struct tracefs_instance *instance,

I wonder is it really necessary this function to take 'tracefs_instance' as argument?

I remember from conversations with Steven that setting the histograms/triggers in the top instance will have the same 
effect. Or maybe I am wrong? The same question applies for the 'destroy' API.

Thanks!
Yordan


>   			 struct tracefs_synth *synth)
>   {
> -	char *synthetic_event;
>   	char *start_hist = NULL;
>   	char *end_hist = NULL;
>   	int ret;
> @@ -1937,14 +1928,10 @@ int tracefs_synth_create(struct tracefs_instance *instance,
>   	if (verify_state(synth) < 0)
>   		return -1;
>   
> -	synthetic_event = create_synthetic_event(synth);
> -	if (!synthetic_event)
> +	if (!synth->dyn_event && alloc_synthetic_event(synth))
> +		return -1;
> +	if (!tracefs_dynevent_create(synth->dyn_event))
>   		return -1;
> -
> -	ret = tracefs_instance_file_append(NULL, "synthetic_events",
> -					   synthetic_event);
> -	if (ret < 0)
> -		goto free_synthetic;
>   
>   	start_hist = create_hist(synth->start_keys, synth->start_vars);
>   	start_hist = append_filter(start_hist, synth->start_filter,
> @@ -1980,9 +1967,7 @@ int tracefs_synth_create(struct tracefs_instance *instance,
>    remove_synthetic:
>   	free(end_hist);
>   	free(start_hist);
> -	remove_synthetic(synthetic_event);
> - free_synthetic:
> -	free(synthetic_event);
> +	tracefs_dynevent_destroy(synth->dyn_event, false);
>   	return -1;
>   }
>   
> @@ -2007,7 +1992,6 @@ int tracefs_synth_create(struct tracefs_instance *instance,
>   int tracefs_synth_destroy(struct tracefs_instance *instance,
>   			  struct tracefs_synth *synth)
>   {
> -	char *synthetic_event;
>   	char *hist;
>   	int ret;
>   
> @@ -2041,11 +2025,7 @@ int tracefs_synth_destroy(struct tracefs_instance *instance,
>   	ret = remove_hist(instance, synth->start_event, hist);
>   	free(hist);
>   
> -	synthetic_event = create_synthetic_event(synth);
> -	if (!synthetic_event)
> -		return -1;
> -
> -	ret = remove_synthetic(synthetic_event);
> +	ret = tracefs_dynevent_destroy(synth->dyn_event, false);
>   
>   	return ret ? -1 : 0;
>   }
> @@ -2067,7 +2047,7 @@ int tracefs_synth_show(struct trace_seq *seq,
>   		       struct tracefs_instance *instance,
>   		       struct tracefs_synth *synth)
>   {
> -	char *synthetic_event = NULL;
> +	bool new_event = false;
>   	char *hist = NULL;
>   	char *path;
>   	int ret = -1;
> @@ -2082,16 +2062,19 @@ int tracefs_synth_show(struct trace_seq *seq,
>   		return -1;
>   	}
>   
> -	synthetic_event = create_synthetic_event(synth);
> -	if (!synthetic_event)
> -		return -1;
> +	if (!synth->dyn_event) {
> +		if (alloc_synthetic_event(synth))
> +			return -1;
> +		new_event = true;
> +	}
>   
>   	path = trace_find_tracing_dir();
>   	if (!path)
>   		goto out_free;
>   
> -	trace_seq_printf(seq, "echo '%s' > %s/synthetic_events\n",
> -			 synthetic_event, path);
> +	trace_seq_printf(seq, "echo '%s%s %s' > %s/%s\n",
> +			 synth->dyn_event->prefix, synth->dyn_event->event,
> +			 synth->dyn_event->format, path, synth->dyn_event->trace_file);
>   
>   	tracefs_put_tracing_file(path);
>   	path = tracefs_instance_get_dir(instance);
> @@ -2116,10 +2099,18 @@ int tracefs_synth_show(struct trace_seq *seq,
>   			 hist, path, synth->end_event->system,
>   			 synth->end_event->name);
>   
> +	if (new_event) {
> +		tracefs_dynevent_free(synth->dyn_event);
> +		synth->dyn_event = NULL;
> +	}
> +
>   	ret = 0;
>    out_free:
> -	free(synthetic_event);
>   	free(hist);
>   	tracefs_put_tracing_file(path);
> +	if (new_event) {
> +		tracefs_dynevent_free(synth->dyn_event);
> +		synth->dyn_event = NULL;
> +	}
>   	return ret;
>   }
>