On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add a command to dump statistics per exporter.
Some comments below.
//Eelco
> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> ---
> NEWS | 2 +
> ofproto/ofproto-dpif-lsample.c | 113 +++++++++++++++++++++++++++++++++
> ofproto/ofproto-dpif-lsample.h | 1 +
> ofproto/ofproto-dpif.c | 1 +
> 4 files changed, 117 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 1c05a7120..a443f9fe1 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,8 @@ Post-v3.3.0
> allows samples to be emitted locally (instead of via IPFIX) in a
> datapath-specific manner via the new datapath action called
> "emit_sample".
> Linux kernel datapath is the first to support this feature.
> + A new unixctl command 'lsample/show' shows packet and bytes statistics
> + per local sample exporter.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 0c71e354d..e31dabac4 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -21,7 +21,10 @@
> #include "dpif.h"
> #include "hash.h"
> #include "ofproto.h"
> +#include "ofproto-dpif.h"
> +#include "openvswitch/dynamic-string.h"
> #include "openvswitch/thread.h"
> +#include "unixctl.h"
>
> /* Dpif local sampling.
> *
> @@ -218,3 +221,113 @@ dpif_lsample_unref(struct dpif_lsample *lsample)
> dpif_lsample_destroy(lsample);
> }
> }
> +
> +static int
> +compare_exporter_list(const void *a_, const void *b_)
We are not comparing a list here, so we should choose a more appropriate name,
maybe something like; compare_exporter_collector_set_id(), or if you want it
shorter, maybe comp_exporter_collector_id(), use you fantasy (but not to
much ;).
> +{
> + const struct lsample_exporter_node *a, *b;
> +
> + a = *(struct lsample_exporter_node **)a_;
> + b = *(struct lsample_exporter_node **)b_;
Fix space around casting.
> +
> + if (a->exporter.options.collector_set_id >
> + b->exporter.options.collector_set_id) {
> + return 1;
> + }
> + if (a->exporter.options.collector_set_id <
> + b->exporter.options.collector_set_id) {
> + return -1;
> + }
> + return 0;
> +}
> +
> +static void
> +lsample_exporter_list(struct dpif_lsample *lsample,
> + struct lsample_exporter_node ***list,
> + size_t *num_exporters)
> +{
> + struct lsample_exporter_node *node;
> + struct lsample_exporter_node **exporter_list;
Reverse Christmas tree order.
> + size_t k = 0, n;
> +
> + n = cmap_count(&lsample->exporters);
> +
> + exporter_list = xcalloc(n, sizeof *exporter_list);
> +
> + CMAP_FOR_EACH (node, node, &lsample->exporters) {
> + if (k >= n) {
> + break;
> + }
> + exporter_list[k++] = node;
> + }
> +
> + qsort(exporter_list, k, sizeof *exporter_list, compare_exporter_list);
> +
> + *list = exporter_list;
> + *num_exporters = k;
> +}
> +
> +static void
> +lsample_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> + struct lsample_exporter_node **node_list = NULL;
> + struct lsample_exporter_node *node;
> + struct ds ds = DS_EMPTY_INITIALIZER;
Reverse Christmas tree? Or even better move the *node definition inside the
for loop below.
> + const struct ofproto_dpif *ofproto;
> + size_t i, num;
> +
> + ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> + if (!ofproto) {
> + unixctl_command_reply_error(conn, "no such bridge");
> + return;
> + }
> +
> + if (!ofproto->lsample) {
> + unixctl_command_reply_error(conn, "no local sampling exporters "
> + "configured");
unixctl_command_reply_error(conn,
"no local sampling exporters configured");
> + return;
> + }
> +
> + ds_put_format(&ds, "local sample statistics for bridge \"%s\":\n",
Maybe capital L for Local?
> + argv[1]);
> +
> + lsample_exporter_list(ofproto->lsample, &node_list, &num);
> +
> + for (i = 0; i < num; i++) {
> + uint64_t n_bytes;
> + uint64_t n_packets;
Reverse Christmas tree
> +
> + node = node_list[i];
> +
> + atomic_read_relaxed(&node->exporter.n_packets, &n_packets);
> + atomic_read_relaxed(&node->exporter.n_bytes, &n_bytes);
> +
> + if (i) {
> + ds_put_cstr(&ds, "\n");
> + }
> +
> + ds_put_format(&ds, "- Collector Set ID: %"PRIu32"\n",
> + node->exporter.options.collector_set_id);
> + ds_put_format(&ds, " Local Sample Group: %"PRIu32"\n",
> + node->exporter.options.group_id);
Indentation is off for both format calls above.
> + ds_put_format(&ds, " Total number of bytes: %"PRIu64"\n", n_bytes);
> + ds_put_format(&ds, " Total number of packets: %"PRIu64"\n",
> + n_packets);
On flows we dump packet followed by bytes, so maybe keep the same order.
> + }
> +
It looks like every statistic dump we have has a different format, maybe we
should try to align it a bit? What about mimicking the fdb/stats-show one?
Collector set ID 1:
Group ID : 12
Total packets: 10
Total bytes : 650
> + free(node_list);
> + unixctl_command_reply(conn, ds_cstr(&ds));
> + ds_destroy(&ds);
> +}
> +
> +void dpif_lsample_init(void)
> +{
> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> + if (ovsthread_once_start(&once)) {
> + unixctl_command_register("lsample/show", "bridge", 1, 1,
> + lsample_unixctl_show, NULL);
> + ovsthread_once_done(&once);
> + }
> +}
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index 2ce096161..eb61769f2 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -36,6 +36,7 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
> bool dpif_lsample_get_group_id(struct dpif_lsample *,
> uint32_t collector_set_id,
> uint32_t *group_id);
> +void dpif_lsample_init(void);
Are you aiming for a structured order of functions and new line usage? It
currently seems random. Since this is a new include file, consider adding a
consistent structure.
> void dpif_lsample_credit_stats(struct dpif_lsample *,
> uint32_t collector_set_id,
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index e5a7f0fa5..ccb99c74a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -286,6 +286,7 @@ init(const struct shash *iface_hints)
> ofproto_unixctl_init();
> ofproto_dpif_trace_init();
> udpif_init();
> + dpif_lsample_init();
> }
>
> static void
> --
> 2.45.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
