diff mbox series

[ovs-dev,[RFC] ovn] Add initial Port Binding statistics (pinctrl counts)

Message ID 20200817205300.1266873-1-mmichels@redhat.com
State New
Headers show
Series [ovs-dev,[RFC] ovn] Add initial Port Binding statistics (pinctrl counts) | expand

Commit Message

Mark Michelson Aug. 17, 2020, 8:53 p.m. UTC
This introduces centralized statistics gathering for Port Bindings. In
this initial patch, the only statistics collected are counts of types of
actions handled by pinctrl.

Port_Bindings now have a list of Port_Binding_Statistics added to them.
Each instance of Port_Binding_Statistics on the Port_Binding corresponds
to a specific chassis on which the statistic was captured.

Two ovn-sbctl commands are added which allow for the statistics to be
printed or cleared. The formatting of the output could stand to be
improved.

This patch is being presented as an RFC:

1) Is this sort of table layout appropriate for the task?
2) Are the operations added to pinctrl.c appropriate? In other words, do
   you foresee performance-related issues from the added
   statistics-gathering overhead? The current incarnation triggers an
   ovn-controller run for every added statistic, but it may be more
   appropriate to run every X seconds instead.
3) What other pinctrl-related statistics would be appropriate to add in
   a first patch? Currently, this patch adds only the number of times a
   certain action opcode was handled. There is no individual packet data
   collected, no count of successes vs. errors, no timestamps for when
   the actions were handled, etc.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 controller/pinctrl.c  | 174 ++++++++++++++++++++++++++++++++++++++++++
 controller/pinctrl.h  |   4 +
 include/ovn/actions.h |  37 +++++++++
 lib/actions.c         |  12 +++
 ovn-sb.ovsschema      |  16 +++-
 ovn-sb.xml            |  22 ++++++
 utilities/ovn-sbctl.c |  92 ++++++++++++++++++++++
 7 files changed, 355 insertions(+), 2 deletions(-)
diff mbox series

Patch

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f72ab70e1..b41284093 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -39,6 +39,7 @@ 
 #include "openvswitch/ofp-util.h"
 #include "openvswitch/vlog.h"
 #include "lib/random.h"
+#include "unixctl.h"
 
 #include "lib/dhcp.h"
 #include "ovn-controller.h"
@@ -161,6 +162,7 @@  VLOG_DEFINE_THIS_MODULE(pinctrl);
 static struct ovs_mutex pinctrl_mutex = OVS_MUTEX_INITIALIZER;
 static struct seq *pinctrl_handler_seq;
 static struct seq *pinctrl_main_seq;
+struct hmap pinctrl_stats;
 
 static void *pinctrl_handler(void *arg);
 
@@ -488,6 +490,7 @@  pinctrl_init(void)
     pinctrl.br_int_name = NULL;
     pinctrl_handler_seq = seq_create();
     pinctrl_main_seq = seq_create();
+    hmap_init(&pinctrl_stats);
 
     latch_init(&pinctrl.pinctrl_thread_exit);
     pinctrl.pinctrl_thread = ovs_thread_create("ovn_pinctrl", pinctrl_handler,
@@ -2668,6 +2671,169 @@  exit:
     dp_packet_uninit(pkt_out_ptr);
 }
 
+struct pinctrl_stats_entry {
+    struct hmap_node hmap_node;
+    int64_t count[N_ACTION_OPCODES];
+    int64_t port_key;
+    int64_t dp_key;
+};
+
+static void
+pinctrl_stats_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
+{
+    if (ovnsb_idl_txn && !hmap_is_empty(&pinctrl_stats)) {
+        poll_immediate_wake();
+    }
+}
+
+static void
+flush_pinctrl_stats(void)
+{
+    struct pinctrl_stats_entry *entry;
+    HMAP_FOR_EACH_POP (entry, hmap_node, &pinctrl_stats) {
+        free(entry);
+    }
+}
+
+static void
+pinctrl_stats_destroy(void)
+{
+    flush_pinctrl_stats();
+    hmap_destroy(&pinctrl_stats);
+}
+
+static struct pinctrl_stats_entry *
+find_or_create_pinctrl_stats_entry(int64_t dp_key, uint32_t port_key)
+{
+    uint32_t hash = hash_2words(dp_key, port_key);
+    struct pinctrl_stats_entry *entry;
+
+    HMAP_FOR_EACH_WITH_HASH (entry, hmap_node, hash, &pinctrl_stats) {
+        if (entry->dp_key == dp_key && entry->port_key == port_key) {
+            return entry;
+        }
+    }
+
+    entry = xzalloc(sizeof *entry);
+    entry->dp_key = dp_key;
+    entry->port_key = port_key;
+    hmap_insert(&pinctrl_stats, &entry->hmap_node, hash);
+
+    return entry;
+}
+
+static void
+pinctrl_log_stats(uint32_t opcode, const struct flow *md)
+{
+    int64_t dp_key = ntohll(md->metadata);
+    uint32_t port_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
+
+    struct pinctrl_stats_entry *entry =
+        find_or_create_pinctrl_stats_entry(dp_key, port_key);
+    ovs_assert(entry != NULL);
+    ++entry->count[opcode];
+
+    notify_pinctrl_main();
+}
+
+static void
+get_map_from_pb_stats(const struct sbrec_port_binding_statistics *stats,
+                      struct simap *map)
+{
+    for (size_t i = 0; i < stats->n_pinctrl_ops; i++) {
+        simap_put(map, stats->key_pinctrl_ops[i],
+                  stats->value_pinctrl_ops[i]);
+    }
+}
+
+static const struct sbrec_port_binding_statistics *
+get_pb_chassis_stats(struct ovsdb_idl_txn *ovnsb_idl_txn,
+                     const struct sbrec_port_binding *pb,
+                     const struct sbrec_chassis *chassis)
+{
+    const struct sbrec_port_binding_statistics *stats;
+
+    for (size_t i = 0; i < pb->n_statistics; i++) {
+        stats = pb->statistics[i];
+        if (!strcmp(stats->chassis_name, chassis->name)) {
+            return stats;
+        }
+    }
+
+    /* Stats not found for this chassis. Add it in now */
+    stats = sbrec_port_binding_statistics_insert(ovnsb_idl_txn);
+    sbrec_port_binding_statistics_set_chassis_name(stats, chassis->name);
+
+    struct sbrec_port_binding_statistics **new_stats =
+        xmalloc(sizeof *new_stats * (pb->n_statistics + 1));
+    nullable_memcpy(new_stats, pb->statistics,
+                    sizeof *new_stats * pb->n_statistics);
+    new_stats[pb->n_statistics] =
+        CONST_CAST(struct sbrec_port_binding_statistics *, stats);
+
+    sbrec_port_binding_set_statistics(pb, new_stats, pb->n_statistics + 1);
+    free(new_stats);
+
+    return stats;
+}
+
+static void
+run_pinctrl_stats(struct ovsdb_idl_txn *ovnsb_idl_txn,
+                  struct ovsdb_idl_index *sbrec_port_binding_by_key,
+                  struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
+                  const struct sbrec_chassis *chassis)
+{
+    if (!ovnsb_idl_txn) {
+        return;
+    }
+
+    struct pinctrl_stats_entry *entry;
+    HMAP_FOR_EACH_POP(entry, hmap_node, &pinctrl_stats) {
+        const struct sbrec_port_binding *pb =
+            lport_lookup_by_key(sbrec_datapath_binding_by_key,
+                                sbrec_port_binding_by_key,
+                                entry->dp_key, entry->port_key);
+
+        if (!pb) {
+            free(entry);
+            continue;
+        }
+
+        const struct sbrec_port_binding_statistics *pb_chassis_stats =
+            get_pb_chassis_stats(ovnsb_idl_txn, pb, chassis);
+
+        struct simap pb_stats_map = SIMAP_INITIALIZER(&pb_stats_map);
+        get_map_from_pb_stats(pb_chassis_stats, &pb_stats_map);
+
+        size_t i;
+        for (i = 0; i < N_ACTION_OPCODES; i++) {
+            simap_increase(&pb_stats_map, get_action_opcode_name(i),
+                           entry->count[i]);
+        }
+
+        size_t num_stats = simap_count(&pb_stats_map);
+        const char **ops = xmalloc(num_stats * sizeof *ops);
+        int64_t *counts = xmalloc(num_stats * sizeof *counts);
+
+        struct simap_node *node;
+        i = 0;
+        SIMAP_FOR_EACH (node, &pb_stats_map) {
+            ops[i] = node->name;
+            counts[i] = node->data;
+            i++;
+        }
+
+        sbrec_port_binding_statistics_set_pinctrl_ops(pb_chassis_stats, ops,
+                                                      counts,
+                                                      num_stats);
+
+        simap_destroy(&pb_stats_map);
+        free(ops);
+        free(counts);
+        free(entry);
+    }
+}
+
 /* Called with in the pinctrl_handler thread context. */
 static void
 process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
@@ -2818,6 +2984,10 @@  process_packet_in(struct rconn *swconn, const struct ofp_header *msg)
                      ntohl(ah->opcode));
         break;
     }
+
+    ovs_mutex_lock(&pinctrl_mutex);
+    pinctrl_log_stats(ntohl(ah->opcode), &pin.flow_metadata.flow);
+    ovs_mutex_unlock(&pinctrl_mutex);
 }
 
 /* Called with in the pinctrl_handler thread context. */
@@ -3020,6 +3190,8 @@  pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
                          local_datapaths);
     sync_svc_monitors(ovnsb_idl_txn, svc_mon_table, sbrec_port_binding_by_name,
                       chassis);
+    run_pinctrl_stats(ovnsb_idl_txn, sbrec_port_binding_by_key,
+                      sbrec_datapath_binding_by_key, chassis);
     ovs_mutex_unlock(&pinctrl_mutex);
 }
 
@@ -3543,6 +3715,7 @@  pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
     wait_put_vport_bindings(ovnsb_idl_txn);
     int64_t new_seq = seq_read(pinctrl_main_seq);
     seq_wait(pinctrl_main_seq, new_seq);
+    pinctrl_stats_wait(ovnsb_idl_txn);
 }
 
 /* Called by ovn-controller. */
@@ -3565,6 +3738,7 @@  pinctrl_destroy(void)
     destroy_svc_monitors();
     seq_destroy(pinctrl_main_seq);
     seq_destroy(pinctrl_handler_seq);
+    pinctrl_stats_destroy();
 }
 
 /* Implementation of the "put_arp" and "put_nd" OVN actions.  These
diff --git a/controller/pinctrl.h b/controller/pinctrl.h
index 8fa4baae9..ff0e49d88 100644
--- a/controller/pinctrl.h
+++ b/controller/pinctrl.h
@@ -50,4 +50,8 @@  void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
 void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn);
 void pinctrl_destroy(void);
 
+struct pinctrl_stats_info {
+    struct ovsdb_idl *ovnsb_idl;
+};
+
 #endif /* controller/pinctrl.h */
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 636cb4bc1..8c7b38313 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -455,6 +455,42 @@  void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
 OVNACTS
 #undef OVNACT
 
+#define ACTION_OPCODES \
+    ACTION_OPCODE(ARP, "arp") \
+    ACTION_OPCODE(PUT_ARP, "put_arp") \
+    ACTION_OPCODE(PUT_DHCP_OPTS, "put_dhcp_opts") \
+    ACTION_OPCODE(ND_NA, "nd_na") \
+    ACTION_OPCODE(PUT_ND, "put_nd") \
+    ACTION_OPCODE(PUT_DHCPV6_OPTS, "put_dhcpv6_opts") \
+    ACTION_OPCODE(DNS_LOOKUP, "dns_lookup") \
+    ACTION_OPCODE(LOG, "log") \
+    ACTION_OPCODE(PUT_ND_RA_OPTS, "put_nd_ra_opts") \
+    ACTION_OPCODE(ND_NS, "nd_ns") \
+    ACTION_OPCODE(ICMP, "icmp") \
+    ACTION_OPCODE(TCP_RESET, "tcp_reset") \
+    ACTION_OPCODE(ND_NA_ROUTER, "nd_na_router") \
+    ACTION_OPCODE(PUT_ICMP4_FRAG_MTU, "put_icmp4_frag_mtu") \
+    ACTION_OPCODE(ICMP4_ERROR, "icmp4_error") \
+    ACTION_OPCODE(EVENT, "trigger_event") \
+    ACTION_OPCODE(IGMP, "igmp") \
+    ACTION_OPCODE(BIND_VPORT, "bind_vport") \
+    ACTION_OPCODE(HANDLE_SVC_CHECK, "handle_svc_check") \
+    ACTION_OPCODE(DHCP6_SERVER, "handle_dcpv6_reply") \
+    ACTION_OPCODE(ICMP6_ERROR, "icmp6_error") \
+    ACTION_OPCODE(PUT_ICMP6_FRAG_MTU, "put_icmp6_frag_mtu") 
+
+enum OVS_PACKED_ENUM action_opcode {
+#define ACTION_OPCODE(ENUM, NAME) ACTION_OPCODE_##ENUM,
+    ACTION_OPCODES
+#undef ACTION_OPCODE
+    N_ACTION_OPCODES,
+};
+
+const char *get_action_opcode_name(enum action_opcode opcode);
+
+#if 0
+Old enum definition of opcodes. Serves as good documentation for
+actions and their arguments.
 enum action_opcode {
     /* "arp { ...actions... }".
      *
@@ -606,6 +642,7 @@  enum action_opcode {
      * action header. */
     ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
 };
+#endif
 
 /* Header. */
 struct action_header {
diff --git a/lib/actions.c b/lib/actions.c
index 5fe0a3897..b7e01f85d 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -55,6 +55,18 @@  OVNACTS
 
 /* Helpers. */
 
+const char *action_opcode_names [] = {
+#define ACTION_OPCODE(ENUM, NAME) NAME,
+    ACTION_OPCODES
+#undef ACTION_OPCODE
+};
+
+const char *
+get_action_opcode_name(enum action_opcode opcode)
+{
+    return action_opcode_names[opcode];
+}
+
 /* Implementation of ovnact_put_<ENUM>(). */
 void *
 ovnact_put(struct ofpbuf *ovnacts, enum ovnact_type type, size_t len)
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 3af76540a..152270f10 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
     "version": "2.9.0",
-    "cksum": "223619766 22548",
+    "cksum": "819260930 23105",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -166,6 +166,11 @@ 
                                      "refType": "strong"},
                              "min": 0,
                              "max": "unlimited"}},
+                "statistics": {
+                    "type": {"key": {"type": "uuid",
+                                     "refTable": "Port_Binding_Statistics"},
+                             "min": 0,
+                             "max": "unlimited"}},
                 "ha_chassis_group": {
                     "type": {"key": {"type": "uuid",
                                      "refTable": "HA_Chassis_Group",
@@ -446,6 +451,13 @@ 
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},
             "indexes": [["logical_port", "ip", "port", "protocol"]],
-            "isRoot": true}
+            "isRoot": true},
+        "Port_Binding_Statistics": {
+            "columns": {
+                "chassis_name": {"type": "string"},
+                "pinctrl_ops": {
+                    "type": {"key": "string", "value": "integer",
+                             "min": 0, "max": "unlimited"}}},
+            "isRoot": false}
     }
 }
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 59b21711b..5e77b15d6 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2933,6 +2933,12 @@  tcp.flags = RST;
         VLAN on the physical network. The VLAN ID is used to match
         incoming traffic and is also added to outgoing traffic.
       </column>
+
+      <column name="statistics">
+        An array of <ref table="Port_Binding_Statistics" /> references. A
+        separate row is created for each chassis on which this port binding
+        logs packets in.
+      </column>
     </group>
 
     <group title="VTEP Options">
@@ -4083,4 +4089,20 @@  tcp.flags = RST;
       </column>
     </group>
   </table>
+  <table name="Port_Binding_Statistics">
+    <p>
+      This table stores statistics about port bindings.
+    </p>
+
+    <column name="chassis_name">
+      The chassis on which the statistics were recorded.
+    </column>
+
+    <column name="pinctrl_ops">
+      An array storing the number of times a particular packet in operation
+      has occurred on this port binding. The array indices correspond to
+      internal numeric representations of the operations.
+    </column>
+
+  </table>
 </database>
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 04e082c70..f5a994860 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -54,6 +54,7 @@ 
 #include "timeval.h"
 #include "util.h"
 #include "svec.h"
+#include "ovn/actions.h"
 
 VLOG_DEFINE_THIS_MODULE(sbctl);
 
@@ -1387,6 +1388,95 @@  cmd_set_ssl(struct ctl_context *ctx)
     sbrec_sb_global_set_ssl(sb_global, ssl);
 }
 
+static void
+pre_cmd_print_port_statistics(struct ctl_context *ctx)
+{
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_statistics);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_datapath);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_logical_port);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);
+
+    ovsdb_idl_add_column(ctx->idl, &sbrec_chassis_col_name);
+
+    ovsdb_idl_add_column(ctx->idl, &sbrec_datapath_binding_col_external_ids);
+
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_statistics_col_chassis_name);
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_statistics_col_pinctrl_ops);
+}
+
+static void
+cmd_print_port_statistics(struct ctl_context *ctx)
+{
+    struct ovsdb_idl *ovnsb_idl = ctx->idl;
+
+    const struct sbrec_port_binding *pb;
+    struct shash dps = SHASH_INITIALIZER(&dps);
+
+    struct pb_list_node {
+        const struct sbrec_port_binding *pb;
+        struct ovs_list list;
+    };
+
+    /* Organize stats by datapath */
+    SBREC_PORT_BINDING_FOR_EACH (pb, ovnsb_idl) {
+        if (!pb->n_statistics || !pb->datapath) {
+            continue;
+        }
+        const char *dp_name = smap_get_def(&pb->datapath->external_ids, "name",
+                                           "(none)");
+        struct ovs_list *pb_list;
+        pb_list = shash_find_data(&dps, dp_name);
+        if (!pb_list) {
+            pb_list = xmalloc(sizeof *pb_list);
+            ovs_list_init(pb_list);
+            shash_add(&dps, dp_name, pb_list);
+        }
+        struct pb_list_node *node = xzalloc(sizeof *node);
+        node->pb = pb;
+        ovs_list_push_back(pb_list, &node->list);
+    }
+
+    struct shash_node *node;
+    SHASH_FOR_EACH (node, &dps) {
+        const char *dp_name = node->name;
+        struct ovs_list *pb_list = node->data;
+        ds_put_format(&ctx->output, "%s\n", dp_name);
+        struct pb_list_node *pb_node;
+        LIST_FOR_EACH_POP (pb_node, list, pb_list) {
+            pb = pb_node->pb;
+            ds_put_format(&ctx->output, "  %s\n", pb->logical_port);
+            for (int i = 0; i < pb->n_statistics; i++) {
+                const struct sbrec_port_binding_statistics *stats;
+                stats = pb->statistics[i];
+                ds_put_format(&ctx->output, "    %s\n", stats->chassis_name);
+                for (int j = 0; j < stats->n_pinctrl_ops; j++) {
+                    ds_put_format(&ctx->output, "      %s: %" PRId64 "\n",
+                                  stats->key_pinctrl_ops[j],
+                                  stats->value_pinctrl_ops[j]);
+                }
+            }
+            free(pb_node);
+        }
+    }
+
+    shash_destroy_free_data(&dps);
+}
+static void
+pre_cmd_clear_port_statistics(struct ctl_context *ctx)
+{
+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_statistics);
+}
+
+static void
+cmd_clear_port_statistics(struct ctl_context *ctx)
+{
+    struct ovsdb_idl *ovnsb_idl = ctx->idl;
+    const struct sbrec_port_binding *pb;
+
+    SBREC_PORT_BINDING_FOR_EACH (pb, ovnsb_idl) {
+        sbrec_port_binding_set_statistics(pb, NULL, 0);
+    }
+}
 
 static const struct ctl_table_class tables[SBREC_N_TABLES] = {
     [SBREC_TABLE_CHASSIS].row_ids[0] = {&sbrec_chassis_col_name, NULL, NULL},
@@ -1696,6 +1786,8 @@  static const struct ctl_command_syntax sbctl_commands[] = {
     {"set-ssl", 3, 5,
         "PRIVATE-KEY CERTIFICATE CA-CERT [SSL-PROTOS [SSL-CIPHERS]]",
         pre_cmd_set_ssl, cmd_set_ssl, NULL, "--bootstrap", RW},
+    {"print-port-statistics", 0, 0, "", pre_cmd_print_port_statistics, cmd_print_port_statistics, NULL, "", RO},
+    {"clear-port-statistics", 0, 0, "", pre_cmd_clear_port_statistics, cmd_clear_port_statistics, NULL, "", RW},
 
     {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
 };