qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument
@ 2025-07-30  6:41 Alex Bradbury
  2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

This series contains one minor feature addition and a series of small
bugfixes/improvements. The addition that motivates the submission is to add a
limit argument for the hotblocks plugin, allowing you to control how many
blocks are printed rather than being hardcoded to the 20 most executed.
Setting limit=0 and dumping information about all executed blocks is
incredibly helpful for an analysis script I have downstream.

This is my first contribution to QEMU. I've attempted to follow all of the
guidance in the "Submitting a Patch" guide, but apologies if I missed
anything.

Alex Bradbury (5):
  contrib/plugins/hotblocks: Correctly free sorted counts list
  contrib/plugins/hotblocks: Fix off by one error in iteration of sorted
    blocks
  contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than
    PRId64
  docs/about/emulation: Add documentation for hotblocks plugin arguments
  contrib/plugins/hotblocks: Allow limit to be set as a command line
    argument

 contrib/plugins/hotblocks.c | 20 ++++++++++++++------
 docs/about/emulation.rst    | 12 ++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.50.1



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
@ 2025-07-30  6:41 ` Alex Bradbury
  2025-07-30 14:03   ` Manos Pitsidianakis
  2025-07-30 17:44   ` Pierrick Bouvier
  2025-07-30  6:41 ` [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks Alex Bradbury
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

g_list_free should be passed the head of the list.

Signed-off-by: Alex Bradbury <asb@igalia.com>
---
 contrib/plugins/hotblocks.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 98404b6885..d3dd23ed9f 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -73,15 +73,16 @@ static void exec_count_free(gpointer key, gpointer value, gpointer user_data)
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) report = g_string_new("collected ");
-    GList *counts, *it;
+    GList *counts, *sorted_counts, *it;
     int i;
 
     g_string_append_printf(report, "%d entries in the hash table\n",
                            g_hash_table_size(hotblocks));
     counts = g_hash_table_get_values(hotblocks);
-    it = g_list_sort_with_data(counts, cmp_exec_count, NULL);
+    sorted_counts = g_list_sort_with_data(counts, cmp_exec_count, NULL);
 
-    if (it) {
+    if (sorted_counts) {
+        it = sorted_counts;
         g_string_append_printf(report, "pc, tcount, icount, ecount\n");
 
         for (i = 0; i < limit && it->next; i++, it = it->next) {
@@ -94,7 +95,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
                     qemu_plugin_scoreboard_u64(rec->exec_count)));
         }
 
-        g_list_free(it);
+        g_list_free(sorted_counts);
     }
 
     qemu_plugin_outs(report->str);
-- 
2.50.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
  2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
@ 2025-07-30  6:41 ` Alex Bradbury
  2025-07-30 17:45   ` Pierrick Bouvier
  2025-07-30  6:41 ` [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64 Alex Bradbury
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

The logic to iterate over the hottest blocks will never reach the last
item in the list, as it checks `it->next != NULL` before entering the
loop. It's hard to trigger this off-by-one error with the default
limit=20, but it is a bug and is problematic if that default is changed
to something larger.

Signed-off-by: Alex Bradbury <asb@igalia.com>
---
 contrib/plugins/hotblocks.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index d3dd23ed9f..cf4d6b8c36 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -82,10 +82,9 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     sorted_counts = g_list_sort_with_data(counts, cmp_exec_count, NULL);
 
     if (sorted_counts) {
-        it = sorted_counts;
         g_string_append_printf(report, "pc, tcount, icount, ecount\n");
 
-        for (i = 0; i < limit && it->next; i++, it = it->next) {
+        for (i = 0, it = sorted_counts; i < limit && it; i++, it = it->next) {
             ExecCount *rec = (ExecCount *) it->data;
             g_string_append_printf(
                 report, "0x%016"PRIx64", %d, %ld, %"PRId64"\n",
-- 
2.50.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
  2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
  2025-07-30  6:41 ` [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks Alex Bradbury
@ 2025-07-30  6:41 ` Alex Bradbury
  2025-07-30 13:32   ` Manos Pitsidianakis
  2025-07-30 17:46   ` Pierrick Bouvier
  2025-07-30  6:41 ` [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments Alex Bradbury
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

qemu_plugin_u64_sum returns a uint64_t, so PRIu64 is the correct format
specifier.

Signed-off-by: Alex Bradbury <asb@igalia.com>
---
 contrib/plugins/hotblocks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index cf4d6b8c36..40d8dae1cd 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -87,7 +87,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
         for (i = 0, it = sorted_counts; i < limit && it; i++, it = it->next) {
             ExecCount *rec = (ExecCount *) it->data;
             g_string_append_printf(
-                report, "0x%016"PRIx64", %d, %ld, %"PRId64"\n",
+                report, "0x%016"PRIx64", %d, %ld, %"PRIu64"\n",
                 rec->start_addr, rec->trans_count,
                 rec->insns,
                 qemu_plugin_u64_sum(
-- 
2.50.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
                   ` (2 preceding siblings ...)
  2025-07-30  6:41 ` [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64 Alex Bradbury
@ 2025-07-30  6:41 ` Alex Bradbury
  2025-07-30 17:46   ` Pierrick Bouvier
  2025-07-30  6:41 ` [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument Alex Bradbury
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

Currently just 'inline'.

Signed-off-by: Alex Bradbury <asb@igalia.com>
---
 docs/about/emulation.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst
index 456d01d5b0..9c963f4705 100644
--- a/docs/about/emulation.rst
+++ b/docs/about/emulation.rst
@@ -463,6 +463,16 @@ Example::
   0x000000004002b0, 1, 4, 66087
   ...
 
+Behaviour can be tweaked with the following arguments:
+
+.. list-table:: Hot Blocks plugin arguments
+  :widths: 20 80
+  :header-rows: 1
+
+  * - Option
+    - Description
+  * - inline=true|false
+    - Use faster inline addition of a single counter.
 
 Hot Pages
 .........
-- 
2.50.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
                   ` (3 preceding siblings ...)
  2025-07-30  6:41 ` [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments Alex Bradbury
@ 2025-07-30  6:41 ` Alex Bradbury
  2025-07-30 17:52   ` Pierrick Bouvier
  2025-07-30 13:48 ` [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Manos Pitsidianakis
  2025-07-30 17:48 ` Pierrick Bouvier
  6 siblings, 1 reply; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30  6:41 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bradbury, alex.bennee, erdnaxe, ma.mandourr,
	pierrick.bouvier

Also add documentation for this argument. This allows the default of 20
to be overridden, and is helpful for using the hotblocks plugin for
analysis scripts that require collecting data on a larger number of
blocks (e.g. setting limit=0 to dump information on all blocks).

Signed-off-by: Alex Bradbury <asb@igalia.com>
---
 contrib/plugins/hotblocks.c | 10 +++++++++-
 docs/about/emulation.rst    |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 40d8dae1cd..8ecf033997 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -84,7 +84,8 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     if (sorted_counts) {
         g_string_append_printf(report, "pc, tcount, icount, ecount\n");
 
-        for (i = 0, it = sorted_counts; i < limit && it; i++, it = it->next) {
+        for (i = 0, it = sorted_counts; (limit == 0 || i < limit) && it;
+             i++, it = it->next) {
             ExecCount *rec = (ExecCount *) it->data;
             g_string_append_printf(
                 report, "0x%016"PRIx64", %d, %ld, %"PRIu64"\n",
@@ -170,6 +171,13 @@ int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
                 fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
                 return -1;
             }
+        } else if (g_strcmp0(tokens[0], "limit") == 0) {
+            char *endptr = NULL;
+            limit = g_ascii_strtoull(tokens[1], &endptr, 10);
+            if (endptr == tokens[1] || *endptr != '\0') {
+                fprintf(stderr, "unsigned integer parsing failed: %s\n", opt);
+                return -1;
+            }
         } else {
             fprintf(stderr, "option parsing failed: %s\n", opt);
             return -1;
diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst
index 9c963f4705..185edb8ad7 100644
--- a/docs/about/emulation.rst
+++ b/docs/about/emulation.rst
@@ -473,6 +473,8 @@ Behaviour can be tweaked with the following arguments:
     - Description
   * - inline=true|false
     - Use faster inline addition of a single counter.
+  * - limit=N
+    - The number of blocks to be printed. (Default: N = 20, use 0 for no limit).
 
 Hot Pages
 .........
-- 
2.50.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64
  2025-07-30  6:41 ` [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64 Alex Bradbury
@ 2025-07-30 13:32   ` Manos Pitsidianakis
  2025-07-30 17:46   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Manos Pitsidianakis @ 2025-07-30 13:32 UTC (permalink / raw)
  To: Alex Bradbury
  Cc: qemu-devel, alex.bennee, erdnaxe, ma.mandourr, pierrick.bouvier

On Wed, Jul 30, 2025 at 4:30 PM Alex Bradbury <asb@igalia.com> wrote:
>
> qemu_plugin_u64_sum returns a uint64_t, so PRIu64 is the correct format
> specifier.
>
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>  contrib/plugins/hotblocks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
> index cf4d6b8c36..40d8dae1cd 100644
> --- a/contrib/plugins/hotblocks.c
> +++ b/contrib/plugins/hotblocks.c
> @@ -87,7 +87,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
>          for (i = 0, it = sorted_counts; i < limit && it; i++, it = it->next) {
>              ExecCount *rec = (ExecCount *) it->data;
>              g_string_append_printf(
> -                report, "0x%016"PRIx64", %d, %ld, %"PRId64"\n",
> +                report, "0x%016"PRIx64", %d, %ld, %"PRIu64"\n",
>                  rec->start_addr, rec->trans_count,
>                  rec->insns,
>                  qemu_plugin_u64_sum(
> --
> 2.50.1
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
                   ` (4 preceding siblings ...)
  2025-07-30  6:41 ` [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument Alex Bradbury
@ 2025-07-30 13:48 ` Manos Pitsidianakis
  2025-07-30 13:54   ` Alex Bradbury
  2025-07-30 17:50   ` Pierrick Bouvier
  2025-07-30 17:48 ` Pierrick Bouvier
  6 siblings, 2 replies; 17+ messages in thread
From: Manos Pitsidianakis @ 2025-07-30 13:48 UTC (permalink / raw)
  To: Alex Bradbury
  Cc: qemu-devel, alex.bennee, erdnaxe, ma.mandourr, pierrick.bouvier

Hi Alex,

On Wed, Jul 30, 2025 at 4:19 PM Alex Bradbury <asb@igalia.com> wrote:
>
> This series contains one minor feature addition and a series of small
> bugfixes/improvements. The addition that motivates the submission is to add a
> limit argument for the hotblocks plugin, allowing you to control how many
> blocks are printed rather than being hardcoded to the 20 most executed.
> Setting limit=0 and dumping information about all executed blocks is
> incredibly helpful for an analysis script I have downstream.
>
> This is my first contribution to QEMU. I've attempted to follow all of the
> guidance in the "Submitting a Patch" guide, but apologies if I missed
> anything.
>
> Alex Bradbury (5):
>   contrib/plugins/hotblocks: Correctly free sorted counts list
>   contrib/plugins/hotblocks: Fix off by one error in iteration of sorted
>     blocks
>   contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than
>     PRId64
>   docs/about/emulation: Add documentation for hotblocks plugin arguments
>   contrib/plugins/hotblocks: Allow limit to be set as a command line
>     argument
>
>  contrib/plugins/hotblocks.c | 20 ++++++++++++++------
>  docs/about/emulation.rst    | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 6 deletions(-)
>
> --
> 2.50.1
>
>

I think the mailing list ate up your first patch. Not your fault
though. I did not receive it in my inbox and it's not on
lore.kernel.org either:

> $ b4 shazam cover.1753857212.git.asb@igalia.com
> Grabbing thread from lore.kernel.org/all/cover.1753857212.git.asb@igalia.com/t.mbox.gz
> Checking for newer revisions
> Grabbing search results from lore.kernel.org
> Analyzing 5 messages in the thread
> Looking for additional code-review trailers on lore.kernel.org
> Analyzing 0 code-review messages
> Checking attestation on all messages, may take a moment...
> ---
>   ERROR: missing [1/5]!
>   ✗ [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
>     ✗ BADSIG: DKIM/igalia.com
>   ✗ [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64
>     ✗ BADSIG: DKIM/igalia.com
>   ✗ [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments
>     ✗ BADSIG: DKIM/igalia.com
>   ✗ [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument
>     ✗ BADSIG: DKIM/igalia.com
> ---
> Total patches: 4
> ---
> WARNING: Thread incomplete!
> Applying: contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
> Patch failed at 0001 contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
> error: sha1 information is lacking or useless (contrib/plugins/hotblocks.c).
> error: could not build fake ancestor

Could you resend it?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument
  2025-07-30 13:48 ` [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Manos Pitsidianakis
@ 2025-07-30 13:54   ` Alex Bradbury
  2025-07-30 17:50   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Alex Bradbury @ 2025-07-30 13:54 UTC (permalink / raw)
  To: Manos Pitsidianakis
  Cc: qemu-devel, alex.bennee, erdnaxe, ma.mandourr, pierrick.bouvier

On 2025-07-30 14:48, Manos Pitsidianakis wrote:
> Hi Alex,
> 
> On Wed, Jul 30, 2025 at 4:19 PM Alex Bradbury <asb@igalia.com> wrote:
>>
>> This series contains one minor feature addition and a series of small
>> bugfixes/improvements. The addition that motivates the submission is to add a
>> limit argument for the hotblocks plugin, allowing you to control how many
>> blocks are printed rather than being hardcoded to the 20 most executed.
>> Setting limit=0 and dumping information about all executed blocks is
>> incredibly helpful for an analysis script I have downstream.
>>
>> This is my first contribution to QEMU. I've attempted to follow all of the
>> guidance in the "Submitting a Patch" guide, but apologies if I missed
>> anything.
>>
>> Alex Bradbury (5):
>>   contrib/plugins/hotblocks: Correctly free sorted counts list
>>   contrib/plugins/hotblocks: Fix off by one error in iteration of sorted
>>     blocks
>>   contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than
>>     PRId64
>>   docs/about/emulation: Add documentation for hotblocks plugin arguments
>>   contrib/plugins/hotblocks: Allow limit to be set as a command line
>>     argument
>>
>>  contrib/plugins/hotblocks.c | 20 ++++++++++++++------
>>  docs/about/emulation.rst    | 12 ++++++++++++
>>  2 files changed, 26 insertions(+), 6 deletions(-)
>>
>> --
>> 2.50.1
>>
>>
> 
> I think the mailing list ate up your first patch. Not your fault
> though. I did not receive it in my inbox and it's not on
> lore.kernel.org either:

Thanks for taking a look - it seems like it's made it through now
https://lore.kernel.org/qemu-devel/cf5a00136738b981a12270b76572e8d502daf208.1753857212.git.asb@igalia.com/

Best,

Alex


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list
  2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
@ 2025-07-30 14:03   ` Manos Pitsidianakis
  2025-07-30 17:44   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Manos Pitsidianakis @ 2025-07-30 14:03 UTC (permalink / raw)
  To: Alex Bradbury
  Cc: qemu-devel, alex.bennee, erdnaxe, ma.mandourr, pierrick.bouvier

On Wed, Jul 30, 2025 at 4:50 PM Alex Bradbury <asb@igalia.com> wrote:
>
> g_list_free should be passed the head of the list.
>
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>  contrib/plugins/hotblocks.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
> index 98404b6885..d3dd23ed9f 100644
> --- a/contrib/plugins/hotblocks.c
> +++ b/contrib/plugins/hotblocks.c
> @@ -73,15 +73,16 @@ static void exec_count_free(gpointer key, gpointer value, gpointer user_data)
>  static void plugin_exit(qemu_plugin_id_t id, void *p)
>  {
>      g_autoptr(GString) report = g_string_new("collected ");
> -    GList *counts, *it;
> +    GList *counts, *sorted_counts, *it;
>      int i;
>
>      g_string_append_printf(report, "%d entries in the hash table\n",
>                             g_hash_table_size(hotblocks));
>      counts = g_hash_table_get_values(hotblocks);
> -    it = g_list_sort_with_data(counts, cmp_exec_count, NULL);
> +    sorted_counts = g_list_sort_with_data(counts, cmp_exec_count, NULL);
>
> -    if (it) {
> +    if (sorted_counts) {
> +        it = sorted_counts;
>          g_string_append_printf(report, "pc, tcount, icount, ecount\n");
>
>          for (i = 0; i < limit && it->next; i++, it = it->next) {
> @@ -94,7 +95,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
>                      qemu_plugin_scoreboard_u64(rec->exec_count)));
>          }
>
> -        g_list_free(it);
> +        g_list_free(sorted_counts);
>      }
>
>      qemu_plugin_outs(report->str);
> --
> 2.50.1
>
>

This looks correct to me. `g_hash_table_get_values` documentation says
the returned value must be freed with `g_list_free`, but
`g_list_sort_with_data` might change the list head, so neither
`counts` nor `it` should be freed, but `sorted_counts` which is the
new list head like the commit message says.

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list
  2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
  2025-07-30 14:03   ` Manos Pitsidianakis
@ 2025-07-30 17:44   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:44 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> g_list_free should be passed the head of the list.
> 
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>   contrib/plugins/hotblocks.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

Thanks for catching this.
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
  2025-07-30  6:41 ` [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks Alex Bradbury
@ 2025-07-30 17:45   ` Pierrick Bouvier
  0 siblings, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:45 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> The logic to iterate over the hottest blocks will never reach the last
> item in the list, as it checks `it->next != NULL` before entering the
> loop. It's hard to trigger this off-by-one error with the default
> limit=20, but it is a bug and is problematic if that default is changed
> to something larger.
> 
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>   contrib/plugins/hotblocks.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Thanks for this one too.
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64
  2025-07-30  6:41 ` [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64 Alex Bradbury
  2025-07-30 13:32   ` Manos Pitsidianakis
@ 2025-07-30 17:46   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:46 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> qemu_plugin_u64_sum returns a uint64_t, so PRIu64 is the correct format
> specifier.
> 
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>   contrib/plugins/hotblocks.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments
  2025-07-30  6:41 ` [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments Alex Bradbury
@ 2025-07-30 17:46   ` Pierrick Bouvier
  0 siblings, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:46 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> Currently just 'inline'.
> 
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>   docs/about/emulation.rst | 10 ++++++++++
>   1 file changed, 10 insertions(+)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument
  2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
                   ` (5 preceding siblings ...)
  2025-07-30 13:48 ` [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Manos Pitsidianakis
@ 2025-07-30 17:48 ` Pierrick Bouvier
  6 siblings, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:48 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> This series contains one minor feature addition and a series of small
> bugfixes/improvements. The addition that motivates the submission is to add a
> limit argument for the hotblocks plugin, allowing you to control how many
> blocks are printed rather than being hardcoded to the 20 most executed.
> Setting limit=0 and dumping information about all executed blocks is
> incredibly helpful for an analysis script I have downstream.
> 
> This is my first contribution to QEMU. I've attempted to follow all of the
> guidance in the "Submitting a Patch" guide, but apologies if I missed
> anything.
>

Thanks for contributing Alex.

As you probably read, in case a v2 is expected (the plugins maintainer 
Alex will be back next week, he might have additional comments), you can 
add the "Reviewed-by" tags you already collected to your commits by 
ammending their messages.
For the rest, your series looks good to me.

Regards,
Pierrick

> Alex Bradbury (5):
>    contrib/plugins/hotblocks: Correctly free sorted counts list
>    contrib/plugins/hotblocks: Fix off by one error in iteration of sorted
>      blocks
>    contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than
>      PRId64
>    docs/about/emulation: Add documentation for hotblocks plugin arguments
>    contrib/plugins/hotblocks: Allow limit to be set as a command line
>      argument
> 
>   contrib/plugins/hotblocks.c | 20 ++++++++++++++------
>   docs/about/emulation.rst    | 12 ++++++++++++
>   2 files changed, 26 insertions(+), 6 deletions(-)
> 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument
  2025-07-30 13:48 ` [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Manos Pitsidianakis
  2025-07-30 13:54   ` Alex Bradbury
@ 2025-07-30 17:50   ` Pierrick Bouvier
  1 sibling, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:50 UTC (permalink / raw)
  To: Manos Pitsidianakis, Alex Bradbury
  Cc: qemu-devel, alex.bennee, erdnaxe, ma.mandourr

On 7/30/25 6:48 AM, Manos Pitsidianakis wrote:
> Hi Alex,
> 
> On Wed, Jul 30, 2025 at 4:19 PM Alex Bradbury <asb@igalia.com> wrote:
>>
>> This series contains one minor feature addition and a series of small
>> bugfixes/improvements. The addition that motivates the submission is to add a
>> limit argument for the hotblocks plugin, allowing you to control how many
>> blocks are printed rather than being hardcoded to the 20 most executed.
>> Setting limit=0 and dumping information about all executed blocks is
>> incredibly helpful for an analysis script I have downstream.
>>
>> This is my first contribution to QEMU. I've attempted to follow all of the
>> guidance in the "Submitting a Patch" guide, but apologies if I missed
>> anything.
>>
>> Alex Bradbury (5):
>>    contrib/plugins/hotblocks: Correctly free sorted counts list
>>    contrib/plugins/hotblocks: Fix off by one error in iteration of sorted
>>      blocks
>>    contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than
>>      PRId64
>>    docs/about/emulation: Add documentation for hotblocks plugin arguments
>>    contrib/plugins/hotblocks: Allow limit to be set as a command line
>>      argument
>>
>>   contrib/plugins/hotblocks.c | 20 ++++++++++++++------
>>   docs/about/emulation.rst    | 12 ++++++++++++
>>   2 files changed, 26 insertions(+), 6 deletions(-)
>>
>> --
>> 2.50.1
>>
>>
> 
> I think the mailing list ate up your first patch. Not your fault
> though. I did not receive it in my inbox and it's not on
> lore.kernel.org either:
> 
>> $ b4 shazam cover.1753857212.git.asb@igalia.com
>> Grabbing thread from lore.kernel.org/all/cover.1753857212.git.asb@igalia.com/t.mbox.gz
>> Checking for newer revisions
>> Grabbing search results from lore.kernel.org
>> Analyzing 5 messages in the thread
>> Looking for additional code-review trailers on lore.kernel.org
>> Analyzing 0 code-review messages
>> Checking attestation on all messages, may take a moment...
>> ---
>>    ERROR: missing [1/5]!
>>    ✗ [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
>>      ✗ BADSIG: DKIM/igalia.com
>>    ✗ [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64
>>      ✗ BADSIG: DKIM/igalia.com
>>    ✗ [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments
>>      ✗ BADSIG: DKIM/igalia.com
>>    ✗ [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument
>>      ✗ BADSIG: DKIM/igalia.com
>> ---
>> Total patches: 4
>> ---
>> WARNING: Thread incomplete!
>> Applying: contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
>> Patch failed at 0001 contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks
>> error: sha1 information is lacking or useless (contrib/plugins/hotblocks.c).
>> error: could not build fake ancestor
> 
> Could you resend it?

I've been lucky to receive it, probably because I'm in direct copy as a 
plugins reviewer.
However, it would be better for the mailing list if you could resend it 
indeed Alex (you can apply the reviewed-by at the same time, so this v2 
"adds" something).

Regards,
Pierrick


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument
  2025-07-30  6:41 ` [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument Alex Bradbury
@ 2025-07-30 17:52   ` Pierrick Bouvier
  0 siblings, 0 replies; 17+ messages in thread
From: Pierrick Bouvier @ 2025-07-30 17:52 UTC (permalink / raw)
  To: Alex Bradbury, qemu-devel; +Cc: alex.bennee, erdnaxe, ma.mandourr

On 7/29/25 11:41 PM, Alex Bradbury wrote:
> Also add documentation for this argument. This allows the default of 20
> to be overridden, and is helpful for using the hotblocks plugin for
> analysis scripts that require collecting data on a larger number of
> blocks (e.g. setting limit=0 to dump information on all blocks).
> 
> Signed-off-by: Alex Bradbury <asb@igalia.com>
> ---
>   contrib/plugins/hotblocks.c | 10 +++++++++-
>   docs/about/emulation.rst    |  2 ++
>   2 files changed, 11 insertions(+), 1 deletion(-)

That's a good idea, thanks!
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2025-07-30 19:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-30  6:41 [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Alex Bradbury
2025-07-30  6:41 ` [PATCH 1/5] contrib/plugins/hotblocks: Correctly free sorted counts list Alex Bradbury
2025-07-30 14:03   ` Manos Pitsidianakis
2025-07-30 17:44   ` Pierrick Bouvier
2025-07-30  6:41 ` [PATCH 2/5] contrib/plugins/hotblocks: Fix off by one error in iteration of sorted blocks Alex Bradbury
2025-07-30 17:45   ` Pierrick Bouvier
2025-07-30  6:41 ` [PATCH 3/5] contrib/plugins/hotblocks: Print uint64_t with PRIu64 rather than PRId64 Alex Bradbury
2025-07-30 13:32   ` Manos Pitsidianakis
2025-07-30 17:46   ` Pierrick Bouvier
2025-07-30  6:41 ` [PATCH 4/5] docs/about/emulation: Add documentation for hotblocks plugin arguments Alex Bradbury
2025-07-30 17:46   ` Pierrick Bouvier
2025-07-30  6:41 ` [PATCH 5/5] contrib/plugins/hotblocks: Allow limit to be set as a command line argument Alex Bradbury
2025-07-30 17:52   ` Pierrick Bouvier
2025-07-30 13:48 ` [PATCH 0/5] contrib/plugins/hotblocks: Minor bug fixes and add limit argument Manos Pitsidianakis
2025-07-30 13:54   ` Alex Bradbury
2025-07-30 17:50   ` Pierrick Bouvier
2025-07-30 17:48 ` Pierrick Bouvier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).