On 7/13/26 5:16 PM, Eelco Chaudron wrote:
>
>
> On 17 Jun 2026, at 22:46, Timothy Redaelli via dev wrote:
>
>> When --format json is passed to ovs-appctl, pmd-perf-show returns a
>> JSON object keyed by thread identifier ("pmd-cNN" for PMD threads,
>> "main" for the main thread). Each entry contains:
>>
>> - "core" and "numa" fields (numeric or null)
>> - "measurement-duration-s" (seconds as real)
>> - "iterations" (object with cycle/iteration stats, or null for main)
>> - "rx-packets" (count, kpps, cycles-per-packet)
>> - "datapath-passes" (count, per-packet ratio, all hit types, upcalls)
>> - "tx-packets" (count, kpps, batches, packets-per-batch)
>>
>> Values that depend on TSC cycle accounting (cycles-per-packet,
>> us-per-upcall) are null for the main thread.
>>
>> The JSON formatting function is placed in dpif-netdev-perf.c alongside
>> the text formatter, since both need access to the file-local tsc_hz.
>>
>> Signed-off-by: Timothy Redaelli <[email protected]>
>
> Hi Timothy,
>
> See some comments below. I have copied in Ilya for some of the code
> style questions, as he was planning to review the series anyway.
>
> //Eelco
>
>> ---
>> lib/dpif-netdev-perf.c | 129 +++++++++++++++++++++++++++++++++++++++++
>> lib/dpif-netdev-perf.h | 5 ++
>> lib/dpif-netdev.c | 65 +++++++++++++++++----
>> tests/pmd.at | 78 +++++++++++++++++++++++++
>> 4 files changed, 266 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
>> index 4b3b35852..536d15a07 100644
>> --- a/lib/dpif-netdev-perf.c
>> +++ b/lib/dpif-netdev-perf.c
>> @@ -315,6 +315,135 @@ pmd_perf_format_overall_stats(struct ds *str, struct
>> pmd_perf_stats *s,
>> }
>> }
>>
>> +void
>> +pmd_perf_format_overall_stats_json(struct json *json,
>> + struct pmd_perf_stats *s,
>> + double duration, bool show_iterations)
>> +{
>> + uint64_t stats[PMD_N_STATS];
>> +
>> + if (duration == 0) {
>> + json_object_put(json, "iterations", json_null_create());
>> + json_object_put(json, "rx-packets", json_null_create());
>> + json_object_put(json, "datapath-passes", json_null_create());
>> + json_object_put(json, "tx-packets", json_null_create());
>> + return;
>> + }
>> +
>> + double us_per_cycle = 1000000.0 / tsc_hz;
>> +
>> + pmd_perf_read_counters(s, stats);
>> + uint64_t tot_cycles = stats[PMD_CYCLES_ITER_IDLE] +
>> + stats[PMD_CYCLES_ITER_BUSY];
>> + uint64_t rx_packets = stats[PMD_STAT_RECV];
>> + uint64_t tx_packets = stats[PMD_STAT_SENT_PKTS];
>> + uint64_t tx_batches = stats[PMD_STAT_SENT_BATCHES];
>> + uint64_t passes = stats[PMD_STAT_RECV] +
>> + stats[PMD_STAT_RECIRC];
>> + uint64_t upcalls = stats[PMD_STAT_MISS];
>> + uint64_t upcall_cycles = stats[PMD_CYCLES_UPCALL];
>> + uint64_t tot_iter = histogram_samples(&s->pkts);
>> + uint64_t idle_iter = s->pkts.bin[0];
>> + uint64_t busy_iter = tot_iter >= idle_iter ? tot_iter - idle_iter : 0;
>> + uint64_t sleep_iter = stats[PMD_SLEEP_ITER];
>> + uint64_t tot_sleep_cycles = stats[PMD_CYCLES_SLEEP];
>
> I earlier mentioned I prefer definitions at the top, but given the
> number of variables here I think it is fine as is. Ilya, what is
> your take?
I guess it's fine. A larger concern for me is that we're doing math twice.
A better apporach might be to unconditionally construct JSON, then return
it if the user asked for JSON, or go over the already calculated things
inside the JSON and print them out. This way there will be less room
for errors, and maybe the code will also be a bit more elegant.
One comment for all patches in the set though: do not report null for
keys that do not exist. We should just not report the key. Reporting
the key but with different value types is not a good practice.
>
>> + /* Iterations. */
>> + if (show_iterations) {
>> + struct json *iter = json_object_create();
>
> New line.
>
>> + json_object_put(iter, "total", json_integer_create(tot_iter));
>> + json_object_put(iter, "us-per-iteration",
>> + json_real_create(tot_iter
>> + ? (tot_cycles + tot_sleep_cycles)
>> + * us_per_cycle / tot_iter
>> + : 0));
>> + json_object_put(iter, "used-tsc-cycles",
>> + json_integer_create(tot_cycles));
>> + json_object_put(iter, "used-tsc-pct",
>> + json_real_create(
>> + 100.0 * (tot_cycles / duration) / tsc_hz));
>> + json_object_put(iter, "idle", json_integer_create(idle_iter));
>> + json_object_put(iter, "idle-pct",
>> + json_real_create(tot_cycles
>> + ? 100.0 * stats[PMD_CYCLES_ITER_IDLE] /
>> tot_cycles
>> + : 0));
>> + json_object_put(iter, "busy", json_integer_create(busy_iter));
>> + json_object_put(iter, "busy-pct",
>> + json_real_create(tot_cycles
>> + ? 100.0 * stats[PMD_CYCLES_ITER_BUSY] /
>> tot_cycles
>> + : 0));
>> + json_object_put(iter, "sleep", json_integer_create(sleep_iter));
>> + json_object_put(iter, "sleep-pct",
>> + json_real_create(tot_iter
>> + ? 100.0 * sleep_iter / tot_iter : 0));
>> + json_object_put(iter, "sleep-time-us",
>> + json_real_create(tot_sleep_cycles * us_per_cycle));
>> + json_object_put(iter, "sleep-us-per-iteration",
>> + json_real_create(sleep_iter
>> + ? (tot_sleep_cycles * us_per_cycle) / sleep_iter
>> + : 0));
>
> All these indentations look odd compared to the rest of the code base.
> Mayeb this looks better?
>
> json_object_put(iter, "us-per-iteration",
> json_real_create(
> tot_iter
> ? (tot_cycles + tot_sleep_cycles)
> * us_per_cycle / tot_iter
> : 0));
>
> Ilya, WDYT?
>
>> + json_object_put(json, "iterations", iter);
>> + } else {
>> + json_object_put(json, "iterations", json_null_create());
>> + }
>> +
>> + /* Rx packets. */
>> + struct json *rx = json_object_create();
>
> New line or definition at the top?
>
>> + json_object_put(rx, "count", json_integer_create(rx_packets));
>> + json_object_put(rx, "kpps",
>> + json_real_create(rx_packets
>> + ? (rx_packets / duration) / 1000 : 0));
>> + json_object_put(rx, "cycles-per-packet",
>> + tot_iter && rx_packets
>> + ? json_real_create(
>> + 1.0 * stats[PMD_CYCLES_ITER_BUSY] / rx_packets)
>> + : json_null_create());
>> + json_object_put(json, "rx-packets", rx);
>> +
>> + /* Datapath passes. */
>> + struct json *dp = json_object_create();
>
> New line or definition at the top?
>
>> + json_object_put(dp, "count", json_integer_create(passes));
>> + json_object_put(dp, "passes-per-packet",
>> + json_real_create(rx_packets
>> + ? 1.0 * passes / rx_packets : 0));
>> + json_object_put(dp, "phwol-hits",
>> + json_integer_create(stats[PMD_STAT_PHWOL_HIT]));
>> + json_object_put(dp, "simple-match-hits",
>> + json_integer_create(stats[PMD_STAT_SIMPLE_HIT]));
>> + json_object_put(dp, "emc-hits",
>> + json_integer_create(stats[PMD_STAT_EXACT_HIT]));
>> + json_object_put(dp, "smc-hits",
>> + json_integer_create(stats[PMD_STAT_SMC_HIT]));
>> + json_object_put(dp, "megaflow-hits",
>> + json_integer_create(stats[PMD_STAT_MASKED_HIT]));
>> + json_object_put(dp, "subtable-lookups-per-megaflow-hit",
>> + json_real_create(stats[PMD_STAT_MASKED_HIT]
>> + ? 1.0 * stats[PMD_STAT_MASKED_LOOKUP]
>> + / stats[PMD_STAT_MASKED_HIT]
>> + : 0));
>> + json_object_put(dp, "upcalls", json_integer_create(upcalls));
>> + json_object_put(dp, "us-per-upcall",
>> + tot_iter
>> + ? json_real_create(upcalls
>> + ? (upcall_cycles * us_per_cycle) / upcalls : 0)
>> + : json_null_create());
>> + json_object_put(dp, "lost-upcalls",
>> + json_integer_create(stats[PMD_STAT_LOST]));
>> + json_object_put(json, "datapath-passes", dp);
>> +
>> + /* Tx packets. */
>> + struct json *tx = json_object_create();
>
> New line or definition at the top?
>
>> + json_object_put(tx, "count", json_integer_create(tx_packets));
>> + json_object_put(tx, "kpps",
>> + json_real_create(tx_packets
>> + ? (tx_packets / duration) / 1000 : 0));
>> + json_object_put(tx, "batches", json_integer_create(tx_batches));
>> + json_object_put(tx, "packets-per-batch",
>> + json_real_create(tx_batches
>> + ? 1.0 * tx_packets / tx_batches : 0));
>> + json_object_put(json, "tx-packets", tx);
>> +}
>> +
>> void
>> pmd_perf_format_histograms(struct ds *str, struct pmd_perf_stats *s)
>> {
>> diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h
>> index cf4b6e103..9b0cb789a 100644
>> --- a/lib/dpif-netdev-perf.h
>> +++ b/lib/dpif-netdev-perf.h
>> @@ -29,6 +29,7 @@
>> #include <rte_cycles.h>
>> #endif
>>
>> +#include "openvswitch/json.h"
>> #include "openvswitch/vlog.h"
>> #include "ovs-atomic.h"
>> #include "timeval.h"
>> @@ -423,6 +424,10 @@ struct pmd_perf_params {
>>
>> void pmd_perf_format_overall_stats(struct ds *str, struct pmd_perf_stats *s,
>> double duration, bool format_iterations);
>> +void pmd_perf_format_overall_stats_json(struct json *json,
>
> Just 'struct json *,' is engouht here, same for below
> 'struct pmd_perf_stats *,'.
>> + struct pmd_perf_stats *s,
>> + double duration,
>> + bool show_iterations);
>> void pmd_perf_format_histograms(struct ds *str, struct pmd_perf_stats *s);
>> void pmd_perf_format_iteration_history(struct ds *str,
>> struct pmd_perf_stats *s,
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index b9233ba99..e3b782354 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -641,6 +641,32 @@ format_pmd_thread(struct ds *reply, struct
>> dp_netdev_pmd_thread *pmd)
>> ds_put_cstr(reply, ":\n");
>> }
>>
>> +static struct json *
>> +pmd_info_show_perf_json(struct dp_netdev_pmd_thread *pmd)
>> +{
>> + long long now = time_msec();
>> + double duration = (now - pmd->perf_stats.start_ms) / 1000.0;
>> + bool show_iterations = (pmd->core_id != NON_PMD_CORE_ID);
>> +
> Remove newline
>
>> + struct json *json = json_object_create();
>
> New line
>> + json_object_put(json, "core",
>> + (pmd->core_id != OVS_CORE_UNSPEC
>> + && pmd->core_id != NON_PMD_CORE_ID)
>> + ? json_integer_create(pmd->core_id)
>> + : json_null_create());
>> + json_object_put(json, "numa",
>> + pmd->numa_id != OVS_NUMA_UNSPEC
>> + ? json_integer_create(pmd->numa_id)
>> + : json_null_create());
>> + json_object_put(json, "measurement-duration-s",
>> + json_real_create(duration));
>> +
>> + pmd_perf_format_overall_stats_json(json, &pmd->perf_stats,
>> + duration, show_iterations);
>> +
>> + return json;
>> +}
>> +
>> static void
>> pmd_info_show_perf(struct ds *reply,
>> struct dp_netdev_pmd_thread *pmd,
>> @@ -942,6 +968,7 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int
>> argc, const char *argv[],
>> / INTERVAL_USEC_TO_SEC;
>> bool show_header = true;
>> uint64_t max_sleep;
>> + struct json *json_result = NULL;
>
> Reverse christmas tree.
>
>> ovs_mutex_lock(&dp_netdev_mutex);
>>
>> @@ -981,15 +1008,17 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int
>> argc, const char *argv[],
>>
>> sorted_poll_thread_list(dp, &pmd_list, &n);
>>
>> - if (type == PMD_INFO_SLEEP_SHOW
>> - && unixctl_command_get_output_format(conn)
>> - == UNIXCTL_OUTPUT_FMT_JSON) {
>> - struct json *json_result = pmd_info_sleep_show_json(dp, pmd_list,
>> n);
>> - free(pmd_list);
>> - ovs_mutex_unlock(&dp_netdev_mutex);
>> - unixctl_command_reply_json(conn, json_result);
>> - ds_destroy(&reply);
>> - return;
>> + if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON)
>> {
>> + if (type == PMD_INFO_PERF_SHOW) {
>> + json_result = json_object_create();
>> + } else if (type == PMD_INFO_SLEEP_SHOW) {
>> + json_result = pmd_info_sleep_show_json(dp, pmd_list, n);
>> + free(pmd_list);
>> + ovs_mutex_unlock(&dp_netdev_mutex);
>> + unixctl_command_reply_json(conn, json_result);
>> + ds_destroy(&reply);
>> + return;
>> + }
>> }
>>
>> for (size_t i = 0; i < n; i++) {
>> @@ -1016,7 +1045,17 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int
>> argc, const char *argv[],
>> } else if (type == PMD_INFO_CLEAR_STATS) {
>> pmd_perf_stats_clear(&pmd->perf_stats);
>> } else if (type == PMD_INFO_PERF_SHOW) {
>> - pmd_info_show_perf(&reply, pmd, (struct pmd_perf_params *)aux);
>> + if (json_result) {
>> + bool is_main = pmd->core_id == NON_PMD_CORE_ID;
>> + char *key = is_main
>> + ? xstrdup("main")
>> + : xasprintf("pmd-c%02u", pmd->core_id);
>> + json_object_put_nocopy(json_result, key,
>> + pmd_info_show_perf_json(pmd));
>> + } else {
>> + pmd_info_show_perf(&reply, pmd,
>> + (struct pmd_perf_params *) aux);
>> + }
>
> I would not do this json part here, move all JSON handling to the
> 'if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {'
> branch.
>
>> } else if (type == PMD_INFO_SLEEP_SHOW) {
>> if (show_header) {
>> ds_put_format(&reply, "Default max sleep: %4"PRIu64" us\n",
>> @@ -1032,7 +1071,11 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int
>> argc, const char *argv[],
>>
>> ovs_mutex_unlock(&dp_netdev_mutex);
>>
>> - unixctl_command_reply(conn, ds_cstr(&reply));
>> + if (json_result) {
>> + unixctl_command_reply_json(conn, json_result);
>> + } else {
>> + unixctl_command_reply(conn, ds_cstr(&reply));
>> + }
>> ds_destroy(&reply);
>> }
>>
>> diff --git a/tests/pmd.at b/tests/pmd.at
>> index 79ee2d470..9a2865b8f 100644
>> --- a/tests/pmd.at
>> +++ b/tests/pmd.at
>> @@ -484,6 +484,84 @@ pmd thread numa_id <cleared> core_id <cleared>:
>> Tx batches: 20 (1.00 pkts/batch)
>> ])
>>
>> +dnl Check JSON output.
>> +dnl Mask dynamic values: core id, measurement duration, all iteration
>> fields,
>> +dnl kpps, cycles-per-packet, and us-per-upcall.
>> +AT_CHECK([ovs-appctl --format json --pretty dpif-netdev/pmd-perf-show | dnl
>> + sed 's/"pmd-c[[0-9]][[0-9]]"/"pmd-c<cleared>"/g' | dnl
>> + sed 's/"core": [[0-9]][[0-9]]*/"core": <cleared>/g' | dnl
>> + sed 's/"measurement-duration-s":
>> [[0-9.]][[0-9.]]*/"measurement-duration-s": <cleared>/g' | dnl
>> + sed 's/"kpps": [[0-9.]][[0-9.]]*/"kpps": <cleared>/g' | dnl
>> + sed 's/"cycles-per-packet":
>> [[0-9.]][[0-9.]]*/"cycles-per-packet": <cleared>/g' | dnl
>> + sed 's/"us-per-upcall": [[0-9.]][[0-9.]]*/"us-per-upcall":
>> <cleared>/g' | dnl
>> + sed '/"iterations": {/,/}/{ s/: [[0-9.e+-]][[0-9.e+-]]*/:
>> <cleared>/g; }'], [0], [dnl
>> +{
>> + "main": {
>> + "core": null,
>> + "datapath-passes": {
>> + "count": 0,
>> + "emc-hits": 0,
>> + "lost-upcalls": 0,
>> + "megaflow-hits": 0,
>> + "passes-per-packet": 0,
>> + "phwol-hits": 0,
>> + "simple-match-hits": 0,
>> + "smc-hits": 0,
>> + "subtable-lookups-per-megaflow-hit": 0,
>> + "upcalls": 0,
>> + "us-per-upcall": null},
>> + "iterations": null,
>> + "measurement-duration-s": <cleared>,
>> + "numa": null,
>> + "rx-packets": {
>> + "count": 0,
>> + "cycles-per-packet": null,
>> + "kpps": <cleared>},
>> + "tx-packets": {
>> + "batches": 0,
>> + "count": 0,
>> + "kpps": <cleared>,
>> + "packets-per-batch": 0}},
What happened to the structure we discussed for the stats-show command
in the previous verion of this patch? I'll copy my old suggestion here
as it is relevant to the perf-show output as well:
"packets": "received", "recicrulated"
"flow-cache-hits": "partial-hardware-offload", "simple", "exact",
"signature", "megaflow"
"upcalls": "success", "failure"
"cycles":
"idle": "count", "percentage"
"processing": "count", "percentage"
"averages": "datapath-passes-per-packet", "cycles-per-packet", ...
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
