[ovs-dev,v2,04/10] lflow-cache: Add lflow-cache/show-stats command.

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/lflow-cache.c    |   21 +++++++++++++++++++++
 controller/lflow-cache.h    |    7 +++++++
 controller/ovn-controller.c |   29 +++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)

On Thu, Feb 4, 2021 at 6:55 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Hi Dumitru,

Please see below for  a few comments.

Thanks
Numan

> ---
>  controller/lflow-cache.c    |   21 +++++++++++++++++++++
>  controller/lflow-cache.h    |    7 +++++++
>  controller/ovn-controller.c |   29 +++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
>
> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> index ce129ac..8e1c71e 100644
> --- a/controller/lflow-cache.c
> +++ b/controller/lflow-cache.c
> @@ -21,6 +21,12 @@
>  #include "lflow-cache.h"
>  #include "ovn/expr.h"
>
> +const char *lflow_cache_type_names[LCACHE_T_MAX] = {
> +    [LCACHE_T_CONJ_ID] = "cache-conj-id",
> +    [LCACHE_T_EXPR]    = "cache-expr",
> +    [LCACHE_T_MATCHES] = "cache-matches",
> +};
> +
>  struct lflow_cache {
>      struct hmap entries[LCACHE_T_MAX];
>      bool enabled;
> @@ -103,6 +109,21 @@ lflow_cache_is_enabled(struct lflow_cache *lc)
>      return lc && lc->enabled;
>  }
>
> +struct lflow_cache_stats *
> +lflow_cache_get_stats(const struct lflow_cache *lc)
> +{
> +    if (!lc) {
> +        return NULL;
> +    }
> +
> +    struct lflow_cache_stats *stats = xmalloc(sizeof *stats);
> +
> +    for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> +        stats->n_entries[i] = hmap_count(&lc->entries[i]);
> +    }
> +    return stats;
> +}
> +

Instead of exposing 'struct lflow_cache_stats', I think it's better if
this function
takes a 'struct ds *' as out parameter and fills in all the stat
related details.

This way, there is no need to extern the 'lflow_cache_type_names'.


>  void
>  lflow_cache_add_conj_id(struct lflow_cache *lc,
>                          const struct sbrec_logical_flow *lflow,
> diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h
> index 74a5b81..54873ee 100644
> --- a/controller/lflow-cache.h
> +++ b/controller/lflow-cache.h
> @@ -42,6 +42,8 @@ enum lflow_cache_type {
>      LCACHE_T_NONE = LCACHE_T_MAX, /* Not found in cache. */
>  };
>
> +extern const char *lflow_cache_type_names[LCACHE_T_MAX];
> +
>  struct lflow_cache_value {
>      enum lflow_cache_type type;
>      uint32_t conj_id_ofs;
> @@ -52,11 +54,16 @@ struct lflow_cache_value {
>      };
>  };
>
> +struct lflow_cache_stats {
> +    size_t n_entries[LCACHE_T_MAX];
> +};
> +
>  struct lflow_cache *lflow_cache_create(void);
>  void lflow_cache_flush(struct lflow_cache *);
>  void lflow_cache_destroy(struct lflow_cache *);
>  void lflow_cache_enable(struct lflow_cache *, bool enabled);
>  bool lflow_cache_is_enabled(struct lflow_cache *);
> +struct lflow_cache_stats *lflow_cache_get_stats(const struct lflow_cache *);
>
>  void lflow_cache_add_conj_id(struct lflow_cache *,
>                               const struct sbrec_logical_flow *,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 7d247fd..6ee6119 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -82,6 +82,7 @@ static unixctl_cb_func debug_pause_execution;
>  static unixctl_cb_func debug_resume_execution;
>  static unixctl_cb_func debug_status_execution;
>  static unixctl_cb_func lflow_cache_flush_cmd;
> +static unixctl_cb_func lflow_cache_show_stats_cmd;
>  static unixctl_cb_func debug_delay_nb_cfg_report;
>
>  #define DEFAULT_BRIDGE_NAME "br-int"
> @@ -2662,6 +2663,9 @@ main(int argc, char *argv[])
>      unixctl_command_register("lflow-cache/flush", "", 0, 0,
>                               lflow_cache_flush_cmd,
>                               &flow_output_data->pd);
> +    unixctl_command_register("lflow-cache/show-stats", "", 0, 0,
> +                             lflow_cache_show_stats_cmd,
> +                             &flow_output_data->pd);
>
>      bool reset_ovnsb_idl_min_index = false;
>      unixctl_command_register("sb-cluster-state-reset", "", 0, 0,
> @@ -3270,6 +3274,31 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn OVS_UNUSED,
>  }
>
>  static void
> +lflow_cache_show_stats_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                           const char *argv[] OVS_UNUSED, void *arg_)
> +{
> +    struct flow_output_persistent_data *fo_pd = arg_;
> +    struct lflow_cache *lc = fo_pd->lflow_cache;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +
> +    ds_put_format(&ds, "Enabled: %s\n",
> +                  lflow_cache_is_enabled(lc) ? "true" : "false");
> +
> +    struct lflow_cache_stats *stats = lflow_cache_get_stats(lc);
> +    if (stats) {
> +        for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> +            ds_put_format(&ds, "%-16s: %"PRIuSIZE"\n",
> +                          lflow_cache_type_names[i],
> +                          stats->n_entries[i]);
> +        }
> +    }
> +    unixctl_command_reply(conn, ds_cstr(&ds));
> +
> +    ds_destroy(&ds);
> +    free(stats);
> +}
> +

Please document this command in ovn-controller.8.xml.

Thanks
Numan

> +static void
>  cluster_state_reset_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                 const char *argv[] OVS_UNUSED, void *idl_reset_)
>  {
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>