[RFC PATCH 0/4] Modifications of some 'hist' APIs

[RFC PATCH 0/4] Modifications of some 'hist' APIs

[Yordan Karadzhov (VMware)]

The patch-set contains various minor modifications inspired by my first
experience using these APIs. 

This patch-set applies on top of patch 'libtracefs-Fix-code-indentation'.

Yordan Karadzhov (VMware) (4):
  libtracefs: Add new constructors for histograms
  libtracefs: Transform tracefs_hist_add_sort_key()
  libtracefs: Add new 'hist' APIs
  libtracefs: Remove tracefs_hist_add_key()

 Documentation/libtracefs-hist-cont.txt |   8 +-
 Documentation/libtracefs-hist.txt      |   8 +-
 include/tracefs.h                      |  27 +++-
 src/tracefs-hist.c                     | 173 ++++++++++++++++++-------
 4 files changed, 159 insertions(+), 57 deletions(-)

-- 
2.30.2

[RFC PATCH 1/4] libtracefs: Add new constructors for histograms

[Yordan Karadzhov (VMware)]

The current way to create an N-dimensional histogram is bit
counterintuitive. We first have to call tracefs_hist_alloc()
which is essentially a constructor of 1d histogram. Then we
have to call tracefs_hist_add_key() N-1 times in order to
increase the dimentions of the histogram. Here we transform
tracefs_hist_alloc() into a constructor of N-dimensional
histogram and we also add 2 helper constructors for 1d and
2d histograms.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 include/tracefs.h  | 17 +++++++++-
 src/tracefs-hist.c | 82 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/include/tracefs.h b/include/tracefs.h
index b4aa75d..64fbb3f 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -280,10 +280,25 @@ enum tracefs_hist_sort_direction {
 struct tracefs_hist;
 
 void tracefs_hist_free(struct tracefs_hist *hist);
+struct tracefs_hist *
+tracefs_hist1d_alloc(struct tracefs_instance * instance,
+		     const char *system, const char *event,
+		     const char *key, enum tracefs_hist_key_type type);
+struct tracefs_hist *
+tracefs_hist2d_alloc(struct tracefs_instance * instance,
+		     const char *system, const char *event,
+		     const char *key1, enum tracefs_hist_key_type type1,
+		     const char *key2, enum tracefs_hist_key_type type2);
+
+struct tracefs_hist_axis {
+	const char *key;
+	enum tracefs_hist_key_type type;
+};
+
 struct tracefs_hist *
 tracefs_hist_alloc(struct tracefs_instance * instance,
 		   const char *system, const char *event,
-		   const char *key, enum tracefs_hist_key_type type);
+		   struct tracefs_hist_axis *axes);
 int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
 			 enum tracefs_hist_key_type type);
 int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
index 6519f7e..8501d64 100644
--- a/src/tracefs-hist.c
+++ b/src/tracefs-hist.c
@@ -141,7 +141,7 @@ void tracefs_hist_free(struct tracefs_hist *hist)
 }
 
 /**
- * tracefs_hist_alloc - Initialize a histogram
+ * tracefs_hist1d_alloc - Initialize one-dimensional histogram
  * @instance: The instance the histogram will be in (NULL for toplevel)
  * @system: The system the histogram event is in.
  * @event: The event that the histogram will be attached to.
@@ -157,14 +157,69 @@ void tracefs_hist_free(struct tracefs_hist *hist)
  * NULL on failure.
  */
 struct tracefs_hist *
+tracefs_hist1d_alloc(struct tracefs_instance * instance,
+		     const char *system, const char *event,
+		     const char *key, enum tracefs_hist_key_type type)
+{
+	struct tracefs_hist_axis axis[] = {{key, type}, {NULL, 0}};
+
+	return tracefs_hist_alloc(instance, system, event, axis);
+}
+
+/**
+ * tracefs_hist2d_alloc - Initialize two-dimensional histogram
+ * @instance: The instance the histogram will be in (NULL for toplevel)
+ * @system: The system the histogram event is in.
+ * @event: The event that the histogram will be attached to.
+ * @key1: The first primary key the histogram will use
+ * @type1: The format type of the first key.
+ * @key2: The second primary key the histogram will use
+ * @type2: The format type of the second key.
+ *
+ * Will initialize a histogram descriptor that will be attached to
+ * the @system/@event with the given @key1 and @key2 as the primaries.
+ * This only initializes the descriptor, it does not start the histogram
+ * in the kernel.
+ *
+ * Returns an initialized histogram on success.
+ * NULL on failure.
+ */
+struct tracefs_hist *
+tracefs_hist2d_alloc(struct tracefs_instance * instance,
+		     const char *system, const char *event,
+		     const char *key1, enum tracefs_hist_key_type type1,
+		     const char *key2, enum tracefs_hist_key_type type2)
+{
+	struct tracefs_hist_axis axis[] = {{key1, type1},
+					   {key2, type2},
+					   {NULL, 0}};
+
+	return tracefs_hist_alloc(instance, system, event, axis);
+}
+
+/**
+ * tracefs_hist_alloc - Initialize N-dimensional histogram
+ * @instance: The instance the histogram will be in (NULL for toplevel)
+ * @system: The system the histogram event is in
+ * @event: The event that the histogram will be attached to
+ * @axes: An array of histogram axes, terminated by a {NULL, 0} entry
+ *
+ * Will initialize a histogram descriptor that will be attached to
+ * the @system/@event. This only initializes the descriptor with the given
+ * @axes keys as primaries. This only initializes the descriptor, it does
+ * not start the histogram in the kernel.
+ *
+ * Returns an initialized histogram on success.
+ * NULL on failure.
+ */
+struct tracefs_hist *
 tracefs_hist_alloc(struct tracefs_instance * instance,
 		   const char *system, const char *event,
-		   const char *key, enum tracefs_hist_key_type type)
+		   struct tracefs_hist_axis *axes)
 {
 	struct tracefs_hist *hist;
-	int ret;
 
-	if (!system || !event || !key)
+	if (!system || !event)
 		return NULL;
 
 	if (!tracefs_event_file_exists(instance, system, event, HIST_FILE))
@@ -174,8 +229,7 @@ tracefs_hist_alloc(struct tracefs_instance * instance,
 	if (!hist)
 		return NULL;
 
-	ret = trace_get_instance(instance);
-	if (ret < 0) {
+	if (trace_get_instance(instance) < 0) {
 		free(hist);
 		return NULL;
 	}
@@ -184,16 +238,18 @@ tracefs_hist_alloc(struct tracefs_instance * instance,
 
 	hist->system = strdup(system);
 	hist->event = strdup(event);
+	if (!hist->system || !hist->event)
+		goto fail;
 
-	ret = tracefs_hist_add_key(hist, key, type);
-
-	if (!hist->system || !hist->event || ret < 0) {
-		tracefs_hist_free(hist);
-		return NULL;
-	}
-
+	for (; axes && axes->key; axes++)
+		if (tracefs_hist_add_key(hist, axes->key, axes->type) < 0)
+			goto fail;
 
 	return hist;
+
+ fail:
+	tracefs_hist_free(hist);
+	return NULL;
 }
 
 /**
-- 
2.30.2

[RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Yordan Karadzhov (VMware)]

The current version of the API makes it hard to add multiple sort keys
to a histogram. The only way to do this is to use the variadic arguments,
however in order to do this the caller have to know the number of sort
keys at compile time, because the method overwrite all previously added
keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
into two methods - one that overwrite and one that does not.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 include/tracefs.h  |  4 +++-
 src/tracefs-hist.c | 21 +++++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/tracefs.h b/include/tracefs.h
index 64fbb3f..c3fa1d6 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -303,7 +303,9 @@ int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
 			 enum tracefs_hist_key_type type);
 int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
 int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
-			      const char *sort_key, ...);
+			      char *sort_key);
+int tracefs_hist_sort_key(struct tracefs_hist *hist,
+			  const char *sort_key, ...);
 int tracefs_hist_sort_key_direction(struct tracefs_hist *hist,
 				    const char *sort_key,
 				    enum tracefs_hist_sort_direction dir);
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
index 8501d64..2ea90d9 100644
--- a/src/tracefs-hist.c
+++ b/src/tracefs-hist.c
@@ -453,6 +453,23 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
 	return tracefs_list_add(list, sort_key);
 }
 
+int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
+			      char *sort_key)
+{
+	char **list = hist->sort;
+
+	if (!hist || !sort_key)
+		return -1;
+
+	list = add_sort_key(hist, sort_key, hist->sort);
+	if (!list)
+		return -1;
+
+	hist->sort = list;
+
+	return 0;
+}
+
 /**
  * tracefs_hist_add_sort_key - add a key for sorting the histogram
  * @hist: The histogram to add the sort key to
@@ -464,8 +481,8 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
  *
  * Returns 0 on success, -1 on error.
  */
-int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
-			      const char *sort_key, ...)
+int tracefs_hist_sort_key(struct tracefs_hist *hist,
+			  const char *sort_key, ...)
 {
 	char **list = NULL;
 	char **tmp;
-- 
2.30.2

[RFC PATCH 3/4] libtracefs: Add new 'hist' APIs

[Yordan Karadzhov (VMware)]

The new APIs will be valuable in a number of different scenarios.
For example if the user wants to implement a function that does
the readout of a histogram, the only argument that will have to
be passed to this function is the histogram descriptor. The same
applies for the case when the user wants to print an adequate
error message in a case of a failure.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 include/tracefs.h  |  4 ++++
 src/tracefs-hist.c | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/tracefs.h b/include/tracefs.h
index c3fa1d6..255be9b 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -299,6 +299,10 @@ struct tracefs_hist *
 tracefs_hist_alloc(struct tracefs_instance * instance,
 		   const char *system, const char *event,
 		   struct tracefs_hist_axis *axes);
+const char *tracefs_get_hist_name(struct tracefs_hist *hist);
+const char *tracefs_get_hist_event(struct tracefs_hist *hist);
+const char *tracefs_get_hist_system(struct tracefs_hist *hist);
+struct tracefs_instance *tracefs_get_hist_instance(struct tracefs_hist *hist);
 int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
 			 enum tracefs_hist_key_type type);
 int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
index 2ea90d9..175b627 100644
--- a/src/tracefs-hist.c
+++ b/src/tracefs-hist.c
@@ -34,6 +34,26 @@ struct tracefs_hist {
 	int			size;
 };
 
+const char *tracefs_get_hist_name(struct tracefs_hist *hist)
+{
+	return hist ? hist->name : NULL;
+}
+
+struct tracefs_instance *tracefs_get_hist_instance(struct tracefs_hist *hist)
+{
+	return hist ? hist->instance : NULL;
+}
+
+const char *tracefs_get_hist_event(struct tracefs_hist *hist)
+{
+	return hist ? hist->event : NULL;
+}
+
+const char *tracefs_get_hist_system(struct tracefs_hist *hist)
+{
+	return hist ? hist->system : NULL;
+}
+
 enum tracefs_hist_command {
 	HIST_CMD_NONE = 0,
 	HIST_CMD_PAUSE,
-- 
2.30.2

[RFC PATCH 4/4] libtracefs: Remove tracefs_hist_add_key()

[Yordan Karadzhov (VMware)]

Since we have appropriate constructor for N-dimensional histograms,
this method is no longer needed.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 Documentation/libtracefs-hist-cont.txt |   8 +-
 Documentation/libtracefs-hist.txt      |   8 +-
 include/tracefs.h                      |   2 -
 src/tracefs-hist.c                     | 110 +++++++++++--------------
 4 files changed, 58 insertions(+), 70 deletions(-)

diff --git a/Documentation/libtracefs-hist-cont.txt b/Documentation/libtracefs-hist-cont.txt
index 1b0153c..d584b46 100644
--- a/Documentation/libtracefs-hist-cont.txt
+++ b/Documentation/libtracefs-hist-cont.txt
@@ -82,15 +82,15 @@ int main (int argc, char **argv, char **env)
 		exit(-1);
 	}
 
-	hist = tracefs_hist_alloc(instance, "kmem", "kmalloc",
-				  "call_site", TRACEFS_HIST_KEY_SYM);
+	hist = tracefs_hist2d_alloc(instance, "kmem", "kmalloc",
+				    "call_site", TRACEFS_HIST_KEY_SYM,
+				    "bytes_req", 0);
 	if (!hist) {
 		fprintf(stderr, "Failed hist create\n");
 		exit(-1);
 	}
 
-	ret = tracefs_hist_add_key(hist, "bytes_req", 0);
-	ret |= tracefs_hist_add_value(hist, "bytes_alloc");
+	ret = tracefs_hist_add_value(hist, "bytes_alloc");
 	ret |= tracefs_hist_add_sort_key(hist, "bytes_req", "bytes_alloc", NULL);
 
 	ret |= tracefs_hist_sort_key_direction(hist, "bytes_alloc",
diff --git a/Documentation/libtracefs-hist.txt b/Documentation/libtracefs-hist.txt
index 0254c5f..692f488 100644
--- a/Documentation/libtracefs-hist.txt
+++ b/Documentation/libtracefs-hist.txt
@@ -186,15 +186,15 @@ int main (int argc, char **argv, char **env)
 		exit(-1);
 	}
 
-	hist = tracefs_hist_alloc(instance, "kmem", "kmalloc",
-				  "call_site", TRACEFS_HIST_KEY_SYM);
+	hist = tracefs_hist2d_alloc(instance, "kmem", "kmalloc",
+				    "call_site", TRACEFS_HIST_KEY_SYM,
+				    "bytes_req", 0);
 	if (!hist) {
 		fprintf(stderr, "Failed hist create\n");
 		exit(-1);
 	}
 
-	ret = tracefs_hist_add_key(hist, "bytes_req", 0);
-	ret |= tracefs_hist_add_value(hist, "bytes_alloc");
+	ret = tracefs_hist_add_value(hist, "bytes_alloc");
 	ret |= tracefs_hist_add_sort_key(hist, "bytes_req", "bytes_alloc", NULL);
 
 	ret |= tracefs_hist_sort_key_direction(hist, "bytes_alloc",
diff --git a/include/tracefs.h b/include/tracefs.h
index 255be9b..5c193a2 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -303,8 +303,6 @@ const char *tracefs_get_hist_name(struct tracefs_hist *hist);
 const char *tracefs_get_hist_event(struct tracefs_hist *hist);
 const char *tracefs_get_hist_system(struct tracefs_hist *hist);
 struct tracefs_instance *tracefs_get_hist_instance(struct tracefs_hist *hist);
-int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
-			 enum tracefs_hist_key_type type);
 int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
 int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
 			      char *sort_key);
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
index 175b627..b71b0e3 100644
--- a/src/tracefs-hist.c
+++ b/src/tracefs-hist.c
@@ -217,6 +217,55 @@ tracefs_hist2d_alloc(struct tracefs_instance * instance,
 	return tracefs_hist_alloc(instance, system, event, axis);
 }
 
+static int add_hist_key(struct tracefs_hist *hist, const char *key,
+			 enum tracefs_hist_key_type type)
+{
+	bool use_key = false;
+	char *key_type = NULL;
+	char **new_list;
+	int ret;
+
+	switch (type) {
+	case TRACEFS_HIST_KEY_NORMAL:
+		use_key = true;
+		ret = 0;
+		break;
+	case TRACEFS_HIST_KEY_HEX:
+		ret = asprintf(&key_type, "%s.hex", key);
+		break;
+	case TRACEFS_HIST_KEY_SYM:
+		ret = asprintf(&key_type, "%s.sym", key);
+		break;
+	case TRACEFS_HIST_KEY_SYM_OFFSET:
+		ret = asprintf(&key_type, "%s.sym-offset", key);
+		break;
+	case TRACEFS_HIST_KEY_SYSCALL:
+		ret = asprintf(&key_type, "%s.syscall", key);
+		break;
+	case TRACEFS_HIST_KEY_EXECNAME:
+		ret = asprintf(&key_type, "%s.execname", key);
+		break;
+	case TRACEFS_HIST_KEY_LOG:
+		ret = asprintf(&key_type, "%s.log2", key);
+		break;
+	case TRACEFS_HIST_KEY_USECS:
+		ret = asprintf(&key_type, "%s.usecs", key);
+		break;
+	}
+
+	if (ret < 0)
+		return -1;
+
+	new_list = tracefs_list_add(hist->keys, use_key ? key : key_type);
+	free(key_type);
+	if (!new_list)
+		return -1;
+
+	hist->keys = new_list;
+
+	return 0;
+}
+
 /**
  * tracefs_hist_alloc - Initialize N-dimensional histogram
  * @instance: The instance the histogram will be in (NULL for toplevel)
@@ -262,7 +311,7 @@ tracefs_hist_alloc(struct tracefs_instance * instance,
 		goto fail;
 
 	for (; axes && axes->key; axes++)
-		if (tracefs_hist_add_key(hist, axes->key, axes->type) < 0)
+		if (add_hist_key(hist, axes->key, axes->type) < 0)
 			goto fail;
 
 	return hist;
@@ -272,65 +321,6 @@ tracefs_hist_alloc(struct tracefs_instance * instance,
 	return NULL;
 }
 
-/**
- * tracefs_hist_add_key - add to a key to a histogram
- * @hist: The histogram to add the key to.
- * @key: The name of the key field.
- * @type: The type of the key format.
- *
- * This adds a secondary or tertiary key to the histogram.
- *
- * Returns 0 on success, -1 on error.
- */
-int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
-			 enum tracefs_hist_key_type type)
-{
-	bool use_key = false;
-	char *key_type = NULL;
-	char **new_list;
-	int ret;
-
-	switch (type) {
-	case TRACEFS_HIST_KEY_NORMAL:
-		use_key = true;
-		ret = 0;
-		break;
-	case TRACEFS_HIST_KEY_HEX:
-		ret = asprintf(&key_type, "%s.hex", key);
-		break;
-	case TRACEFS_HIST_KEY_SYM:
-		ret = asprintf(&key_type, "%s.sym", key);
-		break;
-	case TRACEFS_HIST_KEY_SYM_OFFSET:
-		ret = asprintf(&key_type, "%s.sym-offset", key);
-		break;
-	case TRACEFS_HIST_KEY_SYSCALL:
-		ret = asprintf(&key_type, "%s.syscall", key);
-		break;
-	case TRACEFS_HIST_KEY_EXECNAME:
-		ret = asprintf(&key_type, "%s.execname", key);
-		break;
-	case TRACEFS_HIST_KEY_LOG:
-		ret = asprintf(&key_type, "%s.log2", key);
-		break;
-	case TRACEFS_HIST_KEY_USECS:
-		ret = asprintf(&key_type, "%s.usecs", key);
-		break;
-	}
-
-	if (ret < 0)
-		return -1;
-
-	new_list = tracefs_list_add(hist->keys, use_key ? key : key_type);
-	free(key_type);
-	if (!new_list)
-		return -1;
-
-	hist->keys = new_list;
-
-	return 0;
-}
-
 /**
  * tracefs_hist_add_value - add to a value to a histogram
  * @hist: The histogram to add the value to.
-- 
2.30.2

Re: [RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Steven Rostedt]

On Fri, 10 Sep 2021 19:38:55 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The current version of the API makes it hard to add multiple sort keys
> to a histogram. The only way to do this is to use the variadic arguments,
> however in order to do this the caller have to know the number of sort
> keys at compile time, because the method overwrite all previously added
> keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
> into two methods - one that overwrite and one that does not.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  include/tracefs.h  |  4 +++-
>  src/tracefs-hist.c | 21 +++++++++++++++++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 64fbb3f..c3fa1d6 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -303,7 +303,9 @@ int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
>  			 enum tracefs_hist_key_type type);
>  int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
>  int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
> -			      const char *sort_key, ...);
> +			      char *sort_key);

Why did you remove the const? The add_sort_key() takes a const and makes a
copy of it.

> +int tracefs_hist_sort_key(struct tracefs_hist *hist,
> +			  const char *sort_key, ...);
>  int tracefs_hist_sort_key_direction(struct tracefs_hist *hist,
>  				    const char *sort_key,
>  				    enum tracefs_hist_sort_direction dir);
> diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
> index 8501d64..2ea90d9 100644
> --- a/src/tracefs-hist.c
> +++ b/src/tracefs-hist.c
> @@ -453,6 +453,23 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>  	return tracefs_list_add(list, sort_key);
>  }
>  

Needs a kerneldoc documentation header.

> +int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
> +			      char *sort_key)
> +{
> +	char **list = hist->sort;
> +
> +	if (!hist || !sort_key)
> +		return -1;
> +
> +	list = add_sort_key(hist, sort_key, hist->sort);
> +	if (!list)
> +		return -1;
> +
> +	hist->sort = list;
> +
> +	return 0;
> +}
> +
>  /**
>   * tracefs_hist_add_sort_key - add a key for sorting the histogram

The above name needs to be updated.

>   * @hist: The histogram to add the sort key to
> @@ -464,8 +481,8 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>   *
>   * Returns 0 on success, -1 on error.
>   */
> -int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
> -			      const char *sort_key, ...)
> +int tracefs_hist_sort_key(struct tracefs_hist *hist,

How about if we call this:

	tracefs_hist_replace_sort_keys()

I think that would be a more intuitive name.

-- Steve

> +			  const char *sort_key, ...)
>  {
>  	char **list = NULL;
>  	char **tmp;

Re: [RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Steven Rostedt]

On Fri, 10 Sep 2021 19:38:55 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The current version of the API makes it hard to add multiple sort keys
> to a histogram. The only way to do this is to use the variadic arguments,
> however in order to do this the caller have to know the number of sort
> keys at compile time, because the method overwrite all previously added
> keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
> into two methods - one that overwrite and one that does not.

If I'm building a histogram via some interactive method, and have created a
histogram instance. How do I add a new key before executing it?

-- Steve

Re: [RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Yordan Karadzhov]

On 10.09.21 г. 23:01, Steven Rostedt wrote:
> On Fri, 10 Sep 2021 19:38:55 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The current version of the API makes it hard to add multiple sort keys
>> to a histogram. The only way to do this is to use the variadic arguments,
>> however in order to do this the caller have to know the number of sort
>> keys at compile time, because the method overwrite all previously added
>> keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
>> into two methods - one that overwrite and one that does not.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   include/tracefs.h  |  4 +++-
>>   src/tracefs-hist.c | 21 +++++++++++++++++++--
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/tracefs.h b/include/tracefs.h
>> index 64fbb3f..c3fa1d6 100644
>> --- a/include/tracefs.h
>> +++ b/include/tracefs.h
>> @@ -303,7 +303,9 @@ int tracefs_hist_add_key(struct tracefs_hist *hist, const char *key,
>>   			 enum tracefs_hist_key_type type);
>>   int tracefs_hist_add_value(struct tracefs_hist *hist, const char *value);
>>   int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> -			      const char *sort_key, ...);
>> +			      char *sort_key);
> 
> Why did you remove the const? The add_sort_key() takes a const and makes a
> copy of it.

This is a mistake. It must be 'const'.

> 
>> +int tracefs_hist_sort_key(struct tracefs_hist *hist,
>> +			  const char *sort_key, ...);
>>   int tracefs_hist_sort_key_direction(struct tracefs_hist *hist,
>>   				    const char *sort_key,
>>   				    enum tracefs_hist_sort_direction dir);
>> diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
>> index 8501d64..2ea90d9 100644
>> --- a/src/tracefs-hist.c
>> +++ b/src/tracefs-hist.c
>> @@ -453,6 +453,23 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>>   	return tracefs_list_add(list, sort_key);
>>   }
>>   
> 
> Needs a kerneldoc documentation header.

OK

> 
>> +int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> +			      char *sort_key)
>> +{
>> +	char **list = hist->sort;
>> +
>> +	if (!hist || !sort_key)
>> +		return -1;
>> +
>> +	list = add_sort_key(hist, sort_key, hist->sort);
>> +	if (!list)
>> +		return -1;
>> +
>> +	hist->sort = list;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * tracefs_hist_add_sort_key - add a key for sorting the histogram
> 
> The above name needs to be updated.
> 
>>    * @hist: The histogram to add the sort key to
>> @@ -464,8 +481,8 @@ add_sort_key(struct tracefs_hist *hist, const char *sort_key, char **list)
>>    *
>>    * Returns 0 on success, -1 on error.
>>    */
>> -int tracefs_hist_add_sort_key(struct tracefs_hist *hist,
>> -			      const char *sort_key, ...)
>> +int tracefs_hist_sort_key(struct tracefs_hist *hist,
> 
> How about if we call this:
> 
> 	tracefs_hist_replace_sort_keys()
> 
> I think that would be a more intuitive name.

The user may call this function with a histogram that has no sort keys added.
So there will be nothing to replace.
What about naming it 'tracefs_hist_set_sort_keys()'?

Thanks a lot!
Yordan

> 
> -- Steve
> 
>> +			  const char *sort_key, ...)
>>   {
>>   	char **list = NULL;
>>   	char **tmp;
> 

Re: [RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Yordan Karadzhov]

On 10.09.21 г. 23:04, Steven Rostedt wrote:
> On Fri, 10 Sep 2021 19:38:55 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The current version of the API makes it hard to add multiple sort keys
>> to a histogram. The only way to do this is to use the variadic arguments,
>> however in order to do this the caller have to know the number of sort
>> keys at compile time, because the method overwrite all previously added
>> keys. The problem is addressed by splitting tracefs_hist_add_sort_key()
>> into two methods - one that overwrite and one that does not.
> 
> If I'm building a histogram via some interactive method, and have created a
> histogram instance. How do I add a new key before executing it?

Is this comment about adding 'key' or about adding 'sort key'?

Thanks!
Yordan

> 
> -- Steve
> 

Re: [RFC PATCH 2/4] libtracefs: Transform tracefs_hist_add_sort_key()

[Steven Rostedt]

On Mon, 13 Sep 2021 15:26:51 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> > How about if we call this:
> > 
> > 	tracefs_hist_replace_sort_keys()
> > 
> > I think that would be a more intuitive name.  
> 
> The user may call this function with a histogram that has no sort keys added.
> So there will be nothing to replace.
> What about naming it 'tracefs_hist_set_sort_keys()'?

Nothing is something to replace?

Or we remove this completely, and have a:

 tracefs_hist_reset_sort_keys()

Which removes all sort keys, and let you to start with a clean plate.
Thus, the above would really just be "reset" followed by "add".

-- Steve