Looking at this patch, I'll analyze it for potential regressions following the review template.
**Git SHA**: 82458c3e436a20d63981385d1a865f58c850d918 **Author**: Eelco Chaudron <[email protected]> **Subject**: dpif-offload: Move offload_stats_get() API to dpif-offload. This patch moves the existing API for querying global datapath-specific offload statistics from dpif to dpif-offload, temporarily removing the feature for dpdk offload but indicating it will be reintroduced in the next patch. > diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h > index 8212ffb00..01161bdba 100644 > --- a/include/openvswitch/netdev.h > +++ b/include/openvswitch/netdev.h > @@ -101,6 +101,7 @@ struct netdev_custom_counter { > > /* Structure representation of custom statistics */ > struct netdev_custom_stats { > + char *label; /* Optional label/description for this set of counters. */ > uint16_t size; > struct netdev_custom_counter *counters; > }; [ ... ] > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 1411c5a31..074e73922 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -33,6 +33,7 @@ > #include "dirs.h" > #include "dpctl.h" > #include "dpif.h" > +#include "dpif-offload.h" > #include "dpif-provider.h" > #include "openvswitch/dynamic-string.h" > #include "flow.h" [ ... ] > +static int > +compare_custom_stats(const void *a, const void *b) > +{ > + const struct netdev_custom_stats *sa = a; > + const struct netdev_custom_stats *sb = b; > + > + return sa->label && sb->label ? strcmp(sa->label, sb->label) > + : (sa->label != NULL) - (sb->label != > NULL); > +} Does this code handle the case where strcmp is called with NULL pointers? The ternary expression checks for both labels being non-NULL before calling strcmp, but the logic looks complex for handling partial NULL cases. > @@ -1597,34 +1598,59 @@ dpctl_del_flows(int argc, const char *argv[], struct > dpctl_params *dpctl_p) > return error; > } > > +static int > +compare_custom_stats(const void *a, const void *b) > +{ > + const struct netdev_custom_stats *sa = a; > + const struct netdev_custom_stats *sb = b; > + > + return sa->label && sb->label ? strcmp(sa->label, sb->label) > + : (sa->label != NULL) - (sb->label != > NULL); > +} > + > static int > dpctl_offload_stats_show(int argc, const char *argv[], > struct dpctl_params *dpctl_p) > { > - struct netdev_custom_stats stats; > + struct netdev_custom_stats *stats_array; > struct dpif *dpif; > + size_t n_stats; > int error; > - size_t i; > > error = opt_dpif_open(argc, argv, dpctl_p, 2, &dpif); > if (error) { > return error; > } > > - memset(&stats, 0, sizeof(stats)); > - error = dpif_offload_stats_get(dpif, &stats); > + error = dpif_offload_stats_get(dpif, &stats_array, &n_stats); > if (error) { > dpctl_error(dpctl_p, error, "retrieving offload statistics"); > goto close_dpif; > } Could stats_array be uninitialized if dpif_offload_stats_get returns 0 but fails to set stats_array properly? The function should ensure stats_array is set to NULL when n_stats is 0. > + if (stats_array && n_stats) { > + qsort(stats_array, n_stats, sizeof *stats_array, > compare_custom_stats); > + } > + > dpctl_print(dpctl_p, "HW Offload stats:\n"); > - for (i = 0; i < stats.size; i++) { > - dpctl_print(dpctl_p, " %s: %6" PRIu64 "\n", > - stats.counters[i].name, stats.counters[i].value); > + for (size_t i = 0; i < n_stats; i++) { > + struct netdev_custom_stats *stats = &stats_array[i]; > + > + if (!stats) { > + continue; > + } Can stats ever be NULL here since it's taking the address of an array element? This check seems unnecessary. > + if (stats->label) { > + dpctl_print(dpctl_p, " %s:\n", stats->label); > + } > + for (size_t j = 0; j < stats->size; j++) { > + dpctl_print(dpctl_p, " %s: %6" PRIu64 "\n", > + stats->counters[j].name, stats->counters[j].value); > + } > + netdev_free_custom_stats_counters(stats); > } > > - netdev_free_custom_stats_counters(&stats); > + free(stats_array); [ ... ] > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 586b589a3..2aeef471f 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4735,101 +4735,6 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op > **ops, size_t n_ops) > } > } > > -static int > -dpif_netdev_offload_stats_get(struct dpif *dpif, > - struct netdev_custom_stats *stats) > -{ [ ... entire function removed ... ] > @@ -9990,7 +9895,6 @@ const struct dpif_class dpif_netdev_class = { > dpif_netdev_flow_dump_thread_destroy, > dpif_netdev_flow_dump_next, > dpif_netdev_operate, > - dpif_netdev_offload_stats_get, > NULL, /* recv_set */ > NULL, /* handlers_set */ > dpif_netdev_number_handlers_required, Does this removal of dpif_netdev_offload_stats_get temporarily break functionality for existing users until the follow-up patch is applied? [ ... ] > diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c > index 2a1663719..331805296 100644 > --- a/lib/dpif-offload-dummy.c > +++ b/lib/dpif-offload-dummy.c > @@ -272,6 +272,24 @@ dpif_offload_dummy_get_debug(const struct dpif_offload > *offload, struct ds *ds, > } > } > > +static int > +dpif_offload_dummy_get_global_stats(const struct dpif_offload *offload_, > + struct netdev_custom_stats *stats) > +{ > + struct dpif_offload_dummy *offload = dpif_offload_dummy_cast(offload_); > + > + /* Add a single counter telling how many ports we are servicing. */ > + stats->label = xstrdup(dpif_offload_name(offload_)); > + stats->size = 1; > + stats->counters = xmalloc(sizeof(struct netdev_custom_counter) * 1); Does this code properly handle allocation failure? While xmalloc should handle this, the pattern could be simplified to xmalloc(sizeof(struct netdev_custom_counter)). > + stats->counters[0].value = dpif_offload_port_mgr_port_count( > + offload->port_mgr); > + ovs_strzcpy(stats->counters[0].name, "Offloaded port count", > + sizeof stats->counters[0].name); > + > + return 0; > +} [ ... ] > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > index a9ccadce9..666382730 100644 > --- a/lib/dpif-offload.c > +++ b/lib/dpif-offload.c > @@ -424,6 +424,48 @@ dpif_offload_get_debug(const struct dpif_offload > *offload, struct ds *ds, > return true; > } > > +int dpif_offload_stats_get(struct dpif *dpif, > + struct netdev_custom_stats **stats_, > + size_t *n_stats) > +{ > + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); > + struct netdev_custom_stats *stats; > + struct dpif_offload *offload; > + size_t n_providers; > + int error = 0; > + > + *n_stats = 0; > + if (!dp_offload || !dpif_offload_is_offload_enabled()) { > + *stats_ = NULL; > + return 0; > + } > + > + n_providers = ovs_list_size(&dp_offload->offload_providers); > + stats = xcalloc(n_providers, sizeof(struct netdev_custom_stats)); Can n_providers be 0, leading to a potential issue with xcalloc(0, ...)? > + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) { > + if (!offload->class->get_global_stats) { > + continue; > + } > + > + error = offload->class->get_global_stats(offload, > &stats[(*n_stats)]); > + if (error) { > + for (int i = 0; i < *n_stats; i++) { > + netdev_free_custom_stats_counters(&stats[i]); > + } > + *n_stats = 0; > + free(stats); > + stats = NULL; > + break; > + } > + > + (*n_stats)++; > + } Does this code handle the case where (*n_stats) might exceed the allocated n_providers size? This could happen if some providers skip the get_global_stats call while others succeed, potentially causing (*n_stats) to be incorrectly managed. > + *stats_ = stats; > + return error; > +} [ ... ] > diff --git a/lib/netdev.c b/lib/netdev.c > index 6b0ea8e11..ee5d68c99 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2440,6 +2440,8 @@ netdev_free_custom_stats_counters(struct > netdev_custom_stats *custom_stats) > custom_stats->counters = NULL; > custom_stats->size = 0; > } > + free(custom_stats->label); > + custom_stats->label = NULL; > } > } [ ... ] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
