On Wed, Jun 26, 2024 at 02:13:40PM GMT, Eelco Chaudron wrote:
> 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.
>
Ack.
> > +
> > + 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.
>
Ack.
> > + 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.
>
Ack.
> > + 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?
>
Ack.
> > + 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
>
Ack.
> > +
> > + 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.
>
Ack.
> > + 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
>
Yeah, it's a bit caotic. This one looks nice. I'll mimic it.
> > + 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.
>
Ack.
> > 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
