[ovs-dev] [PATCH 1/2] dpdk: Add debug appctl to get socket statistics.

[Eelco Chaudron]

On 25 Apr 2021, at 18:28, Eli Britstein wrote:

> New appctl 'dpdk/get-socket-stats' implemented to get result of
> 'rte_malloc_get_socket_stats()' function for socket passed as an
> argument. If no arguments passed, get the results for all available
> sockets.

The information is malloc() stats per cpu socket, so I would rename the 
command to
dpdk/get-malloc-stats (comment in patch 2 for the name there).

> Could be used for debugging.
>
> Signed-off-by: Eli Britstein <elibr at nvidia.com>
> Reviewed-by: Salem Sol <salems at nvidia.com>
> ---
>  NEWS                 |  2 ++
>  lib/dpdk-unixctl.man |  4 +++
>  lib/dpdk.c           | 60 
> ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 95cf922aa..e0c0f0408 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,8 @@ Post-v2.15.0
>       * New option '--no-record-hostname' to disable hostname 
> configuration
>         in ovsdb on startup.
>       * New command 'record-hostname-if-not-set' to update hostname in 
> ovsdb.
> +   - DPDK:
> +     * New debug appctl command 'dpdk/get-socket-stats'.
>
>  v2.15.0 - 15 Feb 2021
> diff --git a/lib/dpdk-unixctl.man b/lib/dpdk-unixctl.man
> index 2d6d576f2..37f02d6c7 100644
> --- a/lib/dpdk-unixctl.man
> +++ b/lib/dpdk-unixctl.man
> @@ -10,5 +10,9 @@ list of words separated by spaces: a word can be 
> either a logging \fBlevel\fR
>  \fBnotice\fR, \fBinfo\fR or \fBdebug\fR) or a \fBpattern\fR matching 
> DPDK
>  components (see \fBdpdk/log-list\fR command on \fBovs\-appctl\fR(8)) 
> separated
>  by a colon from the logging \fBlevel\fR to apply.
> +.IP "\fBdpdk/get-socket-stats\fR [\fIsocket\fR]"
> +Prints the statistics information about DPDK socket \fIsocket\fR.
> +If called without arguments, information of all the available sockets 
> will
> +be printed.
>  .RE
>  .
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 319540394..5a6b2015c 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -25,6 +25,7 @@
>  #include <rte_cpuflags.h>
>  #include <rte_errno.h>
>  #include <rte_log.h>
> +#include <rte_malloc.h>
>  #include <rte_memzone.h>
>  #include <rte_version.h>
>
> @@ -356,6 +357,63 @@ dpdk_unixctl_log_set(struct unixctl_conn *conn, 
> int argc, const char *argv[],
>      unixctl_command_reply(conn, NULL);
>  }
>
> +static void +dpdk_unixctl_get_socket_stats(struct unixctl_conn *conn,
> +                              int argc, const char *argv[],
> +                              void *aux OVS_UNUSED)

Guess function also need to change as we are not getting sockets stats, 
but malloc stats per socket.

> +{
> +    struct rte_malloc_socket_stats socket_stats;
> +    unsigned int socket_max = rte_socket_count();
> +    unsigned int socket_min = 0;
> +    unsigned int socket;
> +    size_t size;
> +    FILE *stream;
> +    char *response = NULL;
> +
> +    if (argc == 2) {
> +        socket_min = atoi(argv[1]);
> +        socket_max = socket_min + 1;
> +    }
> +
> +    stream = open_memstream(&response, &size);
> +    if (!stream) {
> +        response = xasprintf("Unable to open memstream: %s.",
> +                             ovs_strerror(errno));
> +        unixctl_command_reply_error(conn, response);
> +        goto out;

Don’t think we need a stream in this case, see below.
> +    }
> +
> +    for (socket = socket_min; socket < socket_max; socket++) {
> +        if (rte_malloc_get_socket_stats(socket, &socket_stats)) {
> +            response = xasprintf("Unable to get stats for socket %d: 
> %s.",
> +                                 socket, ovs_strerror(errno));
> +            fclose(stream);
> +            unixctl_command_reply_error(conn, response);

If I put in an invalid socket id, the error message is not printed. Did 
not further research, but if I replace “response” here with a const 
string it gets printed.

> +            goto out;
> +        }
> +        fprintf(stream,
> +                "Socket = %d\n"
> +                "heap_totalsz_bytes = %lu\n"
> +                "heap_freesz_bytes = %lu\n"
> +                "greatest_free_size = %lu\n"
> +                "free_count = %u\n"
> +                "alloc_count = %u\n"
> +                "heap_allocsz_bytes = %lu\n", socket,
> +                socket_stats.heap_totalsz_bytes,
> +                socket_stats.heap_freesz_bytes,
> +                socket_stats.greatest_free_size,
> +                socket_stats.free_count,
> +                socket_stats.alloc_count,
> +                socket_stats.heap_allocsz_bytes);

I think here you do not need to put it to a stream, but you should use 
the normal APIs for this, see dpif_netdev_pmd_info() for an example, 
i.e., pass the result to the unixctl_command_reply().

> +    }
> +
> +    fclose(stream);
> +
> +    unixctl_command_reply(conn, response);
> +out:
> +    free(response);
> +}
> +
>  static bool
>  dpdk_init__(const struct smap *ovs_other_config)
>  {
> @@ -525,6 +583,8 @@ dpdk_init__(const struct smap *ovs_other_config)
>                               dpdk_unixctl_mem_stream, rte_log_dump);
>      unixctl_command_register("dpdk/log-set", "{level | 
> pattern:level}", 0,
>                               INT_MAX, dpdk_unixctl_log_set, NULL);
> +    unixctl_command_register("dpdk/get-socket-stats", "[socket]", 0, 
> INT_MAX,
> +                             dpdk_unixctl_get_socket_stats, NULL);
>
>      /* We are called from the main thread here */
>      RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
> -- 
> 2.28.0.2311.g225365fb51
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev