Re: [ovs-dev] [RFC v2 7/9] ofproto-dpif-lsample: Show stats via unixctl.
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
> > ---
> > 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(>exporters);
> > +
> > +exporter_list = xcalloc(n, sizeof *exporter_list);
> > +
> > +CMAP_FOR_EACH (node, node, >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(, "local sample statistics for bridge \"%s\":\n",
>
> Maybe capital L for Local?
>
Ack.
> > + argv[1]);
> > +
> > +lsample_exporter_list(ofproto->lsample, _list, );
> > +
> > +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(>exporter.n_packets, _packets);
> > +atomic_read_relaxed(>exporter.n_bytes, _bytes);
> > +
> > +if (i) {
> > +ds_put_cstr(, "\n");
> > +}
> > +
> > +ds_put_format(, "- Collector Set ID:
Re: [ovs-dev] [RFC v2 7/9] ofproto-dpif-lsample: Show stats via unixctl.
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
> ---
> 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(>exporters);
> +
> +exporter_list = xcalloc(n, sizeof *exporter_list);
> +
> +CMAP_FOR_EACH (node, node, >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(, "local sample statistics for bridge \"%s\":\n",
Maybe capital L for Local?
> + argv[1]);
> +
> +lsample_exporter_list(ofproto->lsample, _list, );
> +
> +for (i = 0; i < num; i++) {
> +uint64_t n_bytes;
> +uint64_t n_packets;
Reverse Christmas tree
> +
> +node = node_list[i];
> +
> +atomic_read_relaxed(>exporter.n_packets, _packets);
> +atomic_read_relaxed(>exporter.n_bytes, _bytes);
> +
> +if (i) {
> +ds_put_cstr(, "\n");
> +}
> +
> +ds_put_format(, "- Collector Set ID: %"PRIu32"\n",
> +node->exporter.options.collector_set_id);
> +ds_put_format(, " Local Sample Group: %"PRIu32"\n",
> +node->exporter.options.group_id);
Indentation is off for both format calls above.
> +ds_put_format(, " Total number of bytes: %"PRIu64"\n", n_bytes);
> +ds_put_format(, " Total number of packets: %"PRIu64"\n",
> +
Re: [ovs-dev] [RFC v2 7/9] ofproto-dpif-lsample: Show stats via unixctl.
Bleep bloop. Greetings Adrian Moreno, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC v2 7/9] ofproto-dpif-lsample: Show stats via unixctl.
Add a command to dump statistics per exporter.
Signed-off-by: Adrian Moreno
---
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_)
+{
+const struct lsample_exporter_node *a, *b;
+
+a = *(struct lsample_exporter_node **)a_;
+b = *(struct lsample_exporter_node **)b_;
+
+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;
+size_t k = 0, n;
+
+n = cmap_count(>exporters);
+
+exporter_list = xcalloc(n, sizeof *exporter_list);
+
+CMAP_FOR_EACH (node, node, >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;
+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");
+return;
+}
+
+ds_put_format(, "local sample statistics for bridge \"%s\":\n",
+ argv[1]);
+
+lsample_exporter_list(ofproto->lsample, _list, );
+
+for (i = 0; i < num; i++) {
+uint64_t n_bytes;
+uint64_t n_packets;
+
+node = node_list[i];
+
+atomic_read_relaxed(>exporter.n_packets, _packets);
+atomic_read_relaxed(>exporter.n_bytes, _bytes);
+
+if (i) {
+ds_put_cstr(, "\n");
+}
+
+ds_put_format(, "- Collector Set ID: %"PRIu32"\n",
+node->exporter.options.collector_set_id);
+ds_put_format(, " Local Sample Group: %"PRIu32"\n",
+node->exporter.options.group_id);
+ds_put_format(, " Total number of bytes: %"PRIu64"\n", n_bytes);
+ds_put_format(, " Total number of packets: %"PRIu64"\n",
+ n_packets);
+}
+
+free(node_list);
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
+
+void dpif_lsample_init(void)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+if (ovsthread_once_start()) {
+unixctl_command_register("lsample/show", "bridge", 1, 1,
+ lsample_unixctl_show, NULL);
+ovsthread_once_done();
+}
+}
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);
void dpif_lsample_credit_stats(struct dpif_lsample *,
