diff mbox series

[v1] eventdev/rx-adapter: add telemetry callbacks

Message ID 20211007125734.2326512-1-ganapati.kundapura@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series [v1] eventdev/rx-adapter: add telemetry callbacks |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-testing warning apply patch failure

Commit Message

Ganapati Kundapura Oct. 7, 2021, 12:57 p.m. UTC 
  Added telemetry callbacks to get Rx adapter stats, reset stats and
to get rx queue config information.

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>

Comments

Jerin Jacob Oct. 11, 2021, 4:14 p.m. UTC | #1 
On Thu, Oct 7, 2021 at 6:27 PM Ganapati Kundapura
<ganapati.kundapura@intel.com> wrote:
>
> Added telemetry callbacks to get Rx adapter stats, reset stats and
> to get rx queue config information.

rx -> Rx

Change the subject to eventdev/rx_adapter

>
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
>
> diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c b/lib/eventdev/rte_event_eth_rx_adapter.c
> index 9ac976c..fa7191c 100644
> --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> @@ -23,6 +23,7 @@
>  #include "eventdev_pmd.h"
>  #include "rte_eventdev_trace.h"
>  #include "rte_event_eth_rx_adapter.h"
> +#include <rte_telemetry.h>

Move this to the above block where all <...h> header files are grouped.


>
>  #define BATCH_SIZE             32
>  #define BLOCK_CNT_THRESHOLD    10
> @@ -2852,6 +2853,7 @@ rte_event_eth_rx_adapter_stats_get(uint8_t id,
>                                struct rte_event_eth_rx_adapter_stats *stats)
>  {
>         struct rte_event_eth_rx_adapter *rx_adapter;
> +       struct rte_eth_event_enqueue_buffer *buf;
>         struct rte_event_eth_rx_adapter_stats dev_stats_sum = { 0 };
>         struct rte_event_eth_rx_adapter_stats dev_stats;
>         struct rte_eventdev *dev;
> @@ -2887,8 +2889,11 @@ rte_event_eth_rx_adapter_stats_get(uint8_t id,
>         if (rx_adapter->service_inited)
>                 *stats = rx_adapter->stats;
>
> +       buf = &rx_adapter->event_enqueue_buffer;
>         stats->rx_packets += dev_stats_sum.rx_packets;
>         stats->rx_enq_count += dev_stats_sum.rx_enq_count;
> +       stats->rx_event_buf_count = buf->count;
> +       stats->rx_event_buf_size = buf->events_size;
>
>         return 0;
>  }
> @@ -3052,3 +3057,146 @@ rte_event_eth_rx_adapter_queue_conf_get(uint8_t id,
>
>         return 0;
>  }
> +
> +#define RXA_ADD_DICT(stats, s) rte_tel_data_add_dict_u64(d, #s, stats.s)
> +
> +static int
> +handle_rxa_stats(const char *cmd __rte_unused,
> +                const char *params,
> +                struct rte_tel_data *d)
> +{
> +       uint8_t rx_adapter_id;
> +       struct rte_event_eth_rx_adapter_stats rx_adptr_stats;
> +
> +       if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +               return -1;
> +
> +       /* Get Rx adapter ID from parameter string */
> +       rx_adapter_id = atoi(params);
> +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
> +
> +       /* Get Rx adapter stats */
> +       if (rte_event_eth_rx_adapter_stats_get(rx_adapter_id,
> +                                              &rx_adptr_stats)) {
> +               RTE_EDEV_LOG_ERR("Failed to get Rx adapter stats\n");
> +               return -1;
> +       }
> +
> +       rte_tel_data_start_dict(d);
> +       rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_packets);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_poll_count);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_dropped);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_retry);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_count);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_size);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_count);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_start_ts);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_block_cycles);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_end_ts);
> +       RXA_ADD_DICT(rx_adptr_stats, rx_intr_packets);
> +
> +       return 0;
> +}
> +
> +static int
> +handle_rxa_stats_reset(const char *cmd __rte_unused,
> +                      const char *params,
> +                      struct rte_tel_data *d __rte_unused)
> +{
> +       uint8_t rx_adapter_id;
> +
> +       if (params == NULL || strlen(params) == 0 || ~isdigit(*params))
> +               return -1;
> +
> +       /* Get Rx adapter ID from parameter string */
> +       rx_adapter_id = atoi(params);
> +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
> +
> +       /* Reset Rx adapter stats */
> +       if (rte_event_eth_rx_adapter_stats_reset(rx_adapter_id)) {
> +               RTE_EDEV_LOG_ERR("Failed to reset Rx adapter stats\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +static int
> +handle_rxa_get_queue_conf(const char *cmd __rte_unused,
> +                         const char *params,
> +                         struct rte_tel_data *d)
> +{
> +       uint8_t rx_adapter_id;
> +       uint16_t rx_queue_id;
> +       int eth_dev_id;
> +       char *token, *l_params;
> +       struct rte_event_eth_rx_adapter_queue_conf queue_conf;
> +
> +       if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +               return -1;
> +
> +       /* Get Rx adapter ID from parameter string */
> +       l_params = strdup(params);
> +       token = strtok(l_params, ",");
> +       rx_adapter_id = strtoul(token, NULL, 10);
> +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
> +
> +       token = strtok(NULL, ",");
> +       if (token == NULL || strlen(token) == 0 || !isdigit(*token))
> +               return -1;
> +
> +       /* Get device ID from parameter string */
> +       eth_dev_id = strtoul(token, NULL, 10);
> +       RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(eth_dev_id, -EINVAL);
> +
> +       token = strtok(NULL, ",");
> +       if (token == NULL || strlen(token) == 0 || !isdigit(*token))
> +               return -1;
> +
> +       /* Get Rx queue ID from parameter string */
> +       rx_queue_id = strtoul(token, NULL, 10);
> +       if (rx_queue_id >= rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
> +               RTE_EDEV_LOG_ERR("Invalid rx queue_id %u", rx_queue_id);
> +               return -EINVAL;
> +       }
> +
> +       token = strtok(NULL, "\0");
> +       if (token != NULL)
> +               RTE_EDEV_LOG_ERR("Extra parameters passed to eventdev"
> +                                " telemetry command, igrnoring");
> +
> +       if (rte_event_eth_rx_adapter_queue_conf_get(rx_adapter_id, eth_dev_id,
> +                                                   rx_queue_id, &queue_conf)) {
> +               RTE_EDEV_LOG_ERR("Failed to get Rx adapter queue config");
> +               return -1;
> +       }
> +
> +       rte_tel_data_start_dict(d);
> +       rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
> +       rte_tel_data_add_dict_u64(d, "eth_dev_id", eth_dev_id);
> +       rte_tel_data_add_dict_u64(d, "rx_queue_id", rx_queue_id);
> +       RXA_ADD_DICT(queue_conf, rx_queue_flags);
> +       RXA_ADD_DICT(queue_conf, servicing_weight);
> +       RXA_ADD_DICT(queue_conf.ev, queue_id);
> +       RXA_ADD_DICT(queue_conf.ev, sched_type);
> +       RXA_ADD_DICT(queue_conf.ev, priority);
> +       RXA_ADD_DICT(queue_conf.ev, flow_id);
> +
> +       return 0;
> +}
> +
> +RTE_INIT(rxa_init_telemetry)
> +{
> +       rte_telemetry_register_cmd("/eventdev/rxa_stats",
> +               handle_rxa_stats,
> +               "Returns Rx adapter stats. Parameter: rx_adapter_id");
> +
> +       rte_telemetry_register_cmd("/eventdev/rxa_stats_reset",
> +               handle_rxa_stats_reset,
> +               "Reset Rx adapter stats. Parameter: rx_adapter_id");
> +
> +       rte_telemetry_register_cmd("/eventdev/rxa_queue_conf",
> +               handle_rxa_get_queue_conf,
> +               "Returns Rx queue config. Parameter: rxa_id, DevID, que_id");
> +}
> diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h b/lib/eventdev/rte_event_eth_rx_adapter.h
> index 70ca427..acabed4 100644
> --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
>         /**< Eventdev enqueue count */
>         uint64_t rx_enq_retry;
>         /**< Eventdev enqueue retry count */
> +       uint64_t rx_event_buf_count;
> +       /**< Rx event buffered count */
> +       uint64_t rx_event_buf_size;


Isn't ABI breakage? CI did not warn this. Isn't this a public structure?



> +       /**< Rx event buffer size */
>         uint64_t rx_dropped;
>         /**< Received packet dropped count */
>         uint64_t rx_enq_start_ts;
> --
> 2.6.4
>
Ganapati Kundapura Oct. 12, 2021, 8:35 a.m. UTC | #2 
Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: 11 October 2021 21:44
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> Cc: dpdk-dev <dev@dpdk.org>; Jayatheerthan, Jay
> <jay.jayatheerthan@intel.com>
> Subject: Re: [PATCH v1] eventdev/rx-adapter: add telemetry callbacks
> 
> On Thu, Oct 7, 2021 at 6:27 PM Ganapati Kundapura
> <ganapati.kundapura@intel.com> wrote:
> >
> > Added telemetry callbacks to get Rx adapter stats, reset stats and to
> > get rx queue config information.
> 
> rx -> Rx
> 
> Change the subject to eventdev/rx_adapter
> 
> >
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> >
> > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
> > b/lib/eventdev/rte_event_eth_rx_adapter.c
> > index 9ac976c..fa7191c 100644
> > --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> > +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> > @@ -23,6 +23,7 @@
> >  #include "eventdev_pmd.h"
> >  #include "rte_eventdev_trace.h"
> >  #include "rte_event_eth_rx_adapter.h"
> > +#include <rte_telemetry.h>
> 
> Move this to the above block where all <...h> header files are grouped.
OK
> 
> 
> >
> >  #define BATCH_SIZE             32
> >  #define BLOCK_CNT_THRESHOLD    10
> > @@ -2852,6 +2853,7 @@ rte_event_eth_rx_adapter_stats_get(uint8_t id,
> >                                struct rte_event_eth_rx_adapter_stats
> > *stats)  {
> >         struct rte_event_eth_rx_adapter *rx_adapter;
> > +       struct rte_eth_event_enqueue_buffer *buf;
> >         struct rte_event_eth_rx_adapter_stats dev_stats_sum = { 0 };
> >         struct rte_event_eth_rx_adapter_stats dev_stats;
> >         struct rte_eventdev *dev;
> > @@ -2887,8 +2889,11 @@ rte_event_eth_rx_adapter_stats_get(uint8_t id,
> >         if (rx_adapter->service_inited)
> >                 *stats = rx_adapter->stats;
> >
> > +       buf = &rx_adapter->event_enqueue_buffer;
> >         stats->rx_packets += dev_stats_sum.rx_packets;
> >         stats->rx_enq_count += dev_stats_sum.rx_enq_count;
> > +       stats->rx_event_buf_count = buf->count;
> > +       stats->rx_event_buf_size = buf->events_size;
> >
> >         return 0;
> >  }
> > @@ -3052,3 +3057,146 @@
> > rte_event_eth_rx_adapter_queue_conf_get(uint8_t id,
> >
> >         return 0;
> >  }
> > +
> > +#define RXA_ADD_DICT(stats, s) rte_tel_data_add_dict_u64(d, #s,
> > +stats.s)
> > +
> > +static int
> > +handle_rxa_stats(const char *cmd __rte_unused,
> > +                const char *params,
> > +                struct rte_tel_data *d) {
> > +       uint8_t rx_adapter_id;
> > +       struct rte_event_eth_rx_adapter_stats rx_adptr_stats;
> > +
> > +       if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> > +               return -1;
> > +
> > +       /* Get Rx adapter ID from parameter string */
> > +       rx_adapter_id = atoi(params);
> > +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id,
> > + -EINVAL);
> > +
> > +       /* Get Rx adapter stats */
> > +       if (rte_event_eth_rx_adapter_stats_get(rx_adapter_id,
> > +                                              &rx_adptr_stats)) {
> > +               RTE_EDEV_LOG_ERR("Failed to get Rx adapter stats\n");
> > +               return -1;
> > +       }
> > +
> > +       rte_tel_data_start_dict(d);
> > +       rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_packets);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_poll_count);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_dropped);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_retry);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_count);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_size);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_count);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_start_ts);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_block_cycles);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_enq_end_ts);
> > +       RXA_ADD_DICT(rx_adptr_stats, rx_intr_packets);
> > +
> > +       return 0;
> > +}
> > +
> > +static int
> > +handle_rxa_stats_reset(const char *cmd __rte_unused,
> > +                      const char *params,
> > +                      struct rte_tel_data *d __rte_unused) {
> > +       uint8_t rx_adapter_id;
> > +
> > +       if (params == NULL || strlen(params) == 0 || ~isdigit(*params))
> > +               return -1;
> > +
> > +       /* Get Rx adapter ID from parameter string */
> > +       rx_adapter_id = atoi(params);
> > +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id,
> > + -EINVAL);
> > +
> > +       /* Reset Rx adapter stats */
> > +       if (rte_event_eth_rx_adapter_stats_reset(rx_adapter_id)) {
> > +               RTE_EDEV_LOG_ERR("Failed to reset Rx adapter stats\n");
> > +               return -1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int
> > +handle_rxa_get_queue_conf(const char *cmd __rte_unused,
> > +                         const char *params,
> > +                         struct rte_tel_data *d) {
> > +       uint8_t rx_adapter_id;
> > +       uint16_t rx_queue_id;
> > +       int eth_dev_id;
> > +       char *token, *l_params;
> > +       struct rte_event_eth_rx_adapter_queue_conf queue_conf;
> > +
> > +       if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> > +               return -1;
> > +
> > +       /* Get Rx adapter ID from parameter string */
> > +       l_params = strdup(params);
> > +       token = strtok(l_params, ",");
> > +       rx_adapter_id = strtoul(token, NULL, 10);
> > +       RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id,
> > + -EINVAL);
> > +
> > +       token = strtok(NULL, ",");
> > +       if (token == NULL || strlen(token) == 0 || !isdigit(*token))
> > +               return -1;
> > +
> > +       /* Get device ID from parameter string */
> > +       eth_dev_id = strtoul(token, NULL, 10);
> > +       RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(eth_dev_id, -EINVAL);
> > +
> > +       token = strtok(NULL, ",");
> > +       if (token == NULL || strlen(token) == 0 || !isdigit(*token))
> > +               return -1;
> > +
> > +       /* Get Rx queue ID from parameter string */
> > +       rx_queue_id = strtoul(token, NULL, 10);
> > +       if (rx_queue_id >= rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
> > +               RTE_EDEV_LOG_ERR("Invalid rx queue_id %u", rx_queue_id);
> > +               return -EINVAL;
> > +       }
> > +
> > +       token = strtok(NULL, "\0");
> > +       if (token != NULL)
> > +               RTE_EDEV_LOG_ERR("Extra parameters passed to eventdev"
> > +                                " telemetry command, igrnoring");
> > +
> > +       if (rte_event_eth_rx_adapter_queue_conf_get(rx_adapter_id,
> eth_dev_id,
> > +                                                   rx_queue_id, &queue_conf)) {
> > +               RTE_EDEV_LOG_ERR("Failed to get Rx adapter queue config");
> > +               return -1;
> > +       }
> > +
> > +       rte_tel_data_start_dict(d);
> > +       rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
> > +       rte_tel_data_add_dict_u64(d, "eth_dev_id", eth_dev_id);
> > +       rte_tel_data_add_dict_u64(d, "rx_queue_id", rx_queue_id);
> > +       RXA_ADD_DICT(queue_conf, rx_queue_flags);
> > +       RXA_ADD_DICT(queue_conf, servicing_weight);
> > +       RXA_ADD_DICT(queue_conf.ev, queue_id);
> > +       RXA_ADD_DICT(queue_conf.ev, sched_type);
> > +       RXA_ADD_DICT(queue_conf.ev, priority);
> > +       RXA_ADD_DICT(queue_conf.ev, flow_id);
> > +
> > +       return 0;
> > +}
> > +
> > +RTE_INIT(rxa_init_telemetry)
> > +{
> > +       rte_telemetry_register_cmd("/eventdev/rxa_stats",
> > +               handle_rxa_stats,
> > +               "Returns Rx adapter stats. Parameter: rx_adapter_id");
> > +
> > +       rte_telemetry_register_cmd("/eventdev/rxa_stats_reset",
> > +               handle_rxa_stats_reset,
> > +               "Reset Rx adapter stats. Parameter: rx_adapter_id");
> > +
> > +       rte_telemetry_register_cmd("/eventdev/rxa_queue_conf",
> > +               handle_rxa_get_queue_conf,
> > +               "Returns Rx queue config. Parameter: rxa_id, DevID,
> > +que_id"); }
> > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h
> > b/lib/eventdev/rte_event_eth_rx_adapter.h
> > index 70ca427..acabed4 100644
> > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
> >         /**< Eventdev enqueue count */
> >         uint64_t rx_enq_retry;
> >         /**< Eventdev enqueue retry count */
> > +       uint64_t rx_event_buf_count;
> > +       /**< Rx event buffered count */
> > +       uint64_t rx_event_buf_size;
> 
> 
> Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
Please confirm if moving the above two members to end of the structure overcomes ABI breakage?
> 
> 
> 
> > +       /**< Rx event buffer size */
> >         uint64_t rx_dropped;
> >         /**< Received packet dropped count */
> >         uint64_t rx_enq_start_ts;
> > --
> > 2.6.4
> >
Jerin Jacob Oct. 12, 2021, 8:47 a.m. UTC | #3 
On Tue, Oct 12, 2021 at 2:05 PM Kundapura, Ganapati
<ganapati.kundapura@intel.com> wrote:
>
> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: 11 October 2021 21:44
> > To: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Jayatheerthan, Jay

> > > +que_id"); }
> > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > index 70ca427..acabed4 100644
> > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
> > >         /**< Eventdev enqueue count */
> > >         uint64_t rx_enq_retry;
> > >         /**< Eventdev enqueue retry count */
> > > +       uint64_t rx_event_buf_count;
> > > +       /**< Rx event buffered count */
> > > +       uint64_t rx_event_buf_size;
> >
> >
> > Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
> Please confirm if moving the above two members to end of the structure overcomes ABI breakage?


+ @Ray Kinsella @Thomas Monjalon  @David Marchand

It will still break the ABI. IMO, Since it is an ABI breaking release
it is OK. If there are no other objections, Please move the variable
to end
of the structure and update release notes for ABI changes.

> >
> >
> >
> > > +       /**< Rx event buffer size */
> > >         uint64_t rx_dropped;
> > >         /**< Received packet dropped count */
> > >         uint64_t rx_enq_start_ts;
> > > --
> > > 2.6.4
> > >
Thomas Monjalon Oct. 12, 2021, 9:10 a.m. UTC | #4 
12/10/2021 10:47, Jerin Jacob:
> On Tue, Oct 12, 2021 at 2:05 PM Kundapura, Ganapati
> <ganapati.kundapura@intel.com> wrote:
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
> > > >         /**< Eventdev enqueue count */
> > > >         uint64_t rx_enq_retry;
> > > >         /**< Eventdev enqueue retry count */
> > > > +       uint64_t rx_event_buf_count;
> > > > +       /**< Rx event buffered count */
> > > > +       uint64_t rx_event_buf_size;
> > >
> > >
> > > Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
> > Please confirm if moving the above two members to end of the structure overcomes ABI breakage?
> 
> 
> + @Ray Kinsella @Thomas Monjalon  @David Marchand
> 
> It will still break the ABI. IMO, Since it is an ABI breaking release
> it is OK. If there are no other objections, Please move the variable
> to end
> of the structure and update release notes for ABI changes.

Why moving since it breaks ABI anyway?
I think you can keep as is.
Jerin Jacob Oct. 12, 2021, 9:26 a.m. UTC | #5 
On Tue, Oct 12, 2021 at 2:40 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 12/10/2021 10:47, Jerin Jacob:
> > On Tue, Oct 12, 2021 at 2:05 PM Kundapura, Ganapati
> > <ganapati.kundapura@intel.com> wrote:
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
> > > > >         /**< Eventdev enqueue count */
> > > > >         uint64_t rx_enq_retry;
> > > > >         /**< Eventdev enqueue retry count */
> > > > > +       uint64_t rx_event_buf_count;
> > > > > +       /**< Rx event buffered count */
> > > > > +       uint64_t rx_event_buf_size;
> > > >
> > > >
> > > > Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
> > > Please confirm if moving the above two members to end of the structure overcomes ABI breakage?
> >
> >
> > + @Ray Kinsella @Thomas Monjalon  @David Marchand
> >
> > It will still break the ABI. IMO, Since it is an ABI breaking release
> > it is OK. If there are no other objections, Please move the variable
> > to end
> > of the structure and update release notes for ABI changes.
>
> Why moving since it breaks ABI anyway?

There is no specific gain in keeping new additions in the middle of structure.

> I think you can keep as is.
>
>
>
Ray Kinsella Oct. 12, 2021, 10:05 a.m. UTC | #6 
On 12/10/2021 10:26, Jerin Jacob wrote:
> On Tue, Oct 12, 2021 at 2:40 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 12/10/2021 10:47, Jerin Jacob:
>>> On Tue, Oct 12, 2021 at 2:05 PM Kundapura, Ganapati
>>> <ganapati.kundapura@intel.com> wrote:
>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>> --- a/lib/eventdev/rte_event_eth_rx_adapter.h
>>>>>> +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
>>>>>> @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
>>>>>>         /**< Eventdev enqueue count */
>>>>>>         uint64_t rx_enq_retry;
>>>>>>         /**< Eventdev enqueue retry count */
>>>>>> +       uint64_t rx_event_buf_count;
>>>>>> +       /**< Rx event buffered count */
>>>>>> +       uint64_t rx_event_buf_size;
>>>>>
>>>>>
>>>>> Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
>>>> Please confirm if moving the above two members to end of the structure overcomes ABI breakage?
>>>
>>>
>>> + @Ray Kinsella @Thomas Monjalon  @David Marchand
>>>
>>> It will still break the ABI. IMO, Since it is an ABI breaking release
>>> it is OK. If there are no other objections, Please move the variable
>>> to end
>>> of the structure and update release notes for ABI changes.
>>
>> Why moving since it breaks ABI anyway?
> 
> There is no specific gain in keeping new additions in the middle of structure.

21.11 is an ABI breaking release, so move it where you like :-)

>> I think you can keep as is.
>>
>>
>>
Ganapati Kundapura Oct. 12, 2021, 10:29 a.m. UTC | #7 
Hi,

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: 12 October 2021 15:35
> To: Jerin Jacob <jerinjacobk@gmail.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Kundapura, Ganapati <ganapati.kundapura@intel.com>; David Marchand
> <david.marchand@redhat.com>; dpdk-dev <dev@dpdk.org>; Jayatheerthan, Jay
> <jay.jayatheerthan@intel.com>
> Subject: Re: [PATCH v1] eventdev/rx-adapter: add telemetry callbacks
> 
> 
> 
> On 12/10/2021 10:26, Jerin Jacob wrote:
> > On Tue, Oct 12, 2021 at 2:40 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> >>
> >> 12/10/2021 10:47, Jerin Jacob:
> >>> On Tue, Oct 12, 2021 at 2:05 PM Kundapura, Ganapati
> >>> <ganapati.kundapura@intel.com> wrote:
> >>>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>>>>> --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> >>>>>> +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> >>>>>> @@ -216,6 +216,10 @@ struct rte_event_eth_rx_adapter_stats {
> >>>>>>         /**< Eventdev enqueue count */
> >>>>>>         uint64_t rx_enq_retry;
> >>>>>>         /**< Eventdev enqueue retry count */
> >>>>>> +       uint64_t rx_event_buf_count;
> >>>>>> +       /**< Rx event buffered count */
> >>>>>> +       uint64_t rx_event_buf_size;
> >>>>>
> >>>>>
> >>>>> Isn't ABI breakage? CI did not warn this. Isn't this a public structure?
> >>>> Please confirm if moving the above two members to end of the structure
> overcomes ABI breakage?
> >>>
> >>>
> >>> + @Ray Kinsella @Thomas Monjalon  @David Marchand
> >>>
> >>> It will still break the ABI. IMO, Since it is an ABI breaking
> >>> release it is OK. If there are no other objections, Please move the
> >>> variable to end of the structure and update release notes for ABI
> >>> changes.
> >>
> >> Why moving since it breaks ABI anyway?
> >
> > There is no specific gain in keeping new additions in the middle of structure.
> 
> 21.11 is an ABI breaking release, so move it where you like :-)
Posted new patch with the new struct members moved to the end of the struct, 
updated release notes and review comments addressed.
> 
> >> I think you can keep as is.
> >>
> >>
> >>
diff mbox series

Patch

diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c b/lib/eventdev/rte_event_eth_rx_adapter.c
index 9ac976c..fa7191c 100644
--- a/lib/eventdev/rte_event_eth_rx_adapter.c
+++ b/lib/eventdev/rte_event_eth_rx_adapter.c
@@ -23,6 +23,7 @@ 
 #include "eventdev_pmd.h"
 #include "rte_eventdev_trace.h"
 #include "rte_event_eth_rx_adapter.h"
+#include <rte_telemetry.h>
 
 #define BATCH_SIZE		32
 #define BLOCK_CNT_THRESHOLD	10
@@ -2852,6 +2853,7 @@  rte_event_eth_rx_adapter_stats_get(uint8_t id,
 			       struct rte_event_eth_rx_adapter_stats *stats)
 {
 	struct rte_event_eth_rx_adapter *rx_adapter;
+	struct rte_eth_event_enqueue_buffer *buf;
 	struct rte_event_eth_rx_adapter_stats dev_stats_sum = { 0 };
 	struct rte_event_eth_rx_adapter_stats dev_stats;
 	struct rte_eventdev *dev;
@@ -2887,8 +2889,11 @@  rte_event_eth_rx_adapter_stats_get(uint8_t id,
 	if (rx_adapter->service_inited)
 		*stats = rx_adapter->stats;
 
+	buf = &rx_adapter->event_enqueue_buffer;
 	stats->rx_packets += dev_stats_sum.rx_packets;
 	stats->rx_enq_count += dev_stats_sum.rx_enq_count;
+	stats->rx_event_buf_count = buf->count;
+	stats->rx_event_buf_size = buf->events_size;
 
 	return 0;
 }
@@ -3052,3 +3057,146 @@  rte_event_eth_rx_adapter_queue_conf_get(uint8_t id,
 
 	return 0;
 }
+
+#define RXA_ADD_DICT(stats, s) rte_tel_data_add_dict_u64(d, #s, stats.s)
+
+static int
+handle_rxa_stats(const char *cmd __rte_unused,
+		 const char *params,
+		 struct rte_tel_data *d)
+{
+	uint8_t rx_adapter_id;
+	struct rte_event_eth_rx_adapter_stats rx_adptr_stats;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get Rx adapter ID from parameter string */
+	rx_adapter_id = atoi(params);
+	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
+
+	/* Get Rx adapter stats */
+	if (rte_event_eth_rx_adapter_stats_get(rx_adapter_id,
+					       &rx_adptr_stats)) {
+		RTE_EDEV_LOG_ERR("Failed to get Rx adapter stats\n");
+		return -1;
+	}
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
+	RXA_ADD_DICT(rx_adptr_stats, rx_packets);
+	RXA_ADD_DICT(rx_adptr_stats, rx_poll_count);
+	RXA_ADD_DICT(rx_adptr_stats, rx_dropped);
+	RXA_ADD_DICT(rx_adptr_stats, rx_enq_retry);
+	RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_count);
+	RXA_ADD_DICT(rx_adptr_stats, rx_event_buf_size);
+	RXA_ADD_DICT(rx_adptr_stats, rx_enq_count);
+	RXA_ADD_DICT(rx_adptr_stats, rx_enq_start_ts);
+	RXA_ADD_DICT(rx_adptr_stats, rx_enq_block_cycles);
+	RXA_ADD_DICT(rx_adptr_stats, rx_enq_end_ts);
+	RXA_ADD_DICT(rx_adptr_stats, rx_intr_packets);
+
+	return 0;
+}
+
+static int
+handle_rxa_stats_reset(const char *cmd __rte_unused,
+		       const char *params,
+		       struct rte_tel_data *d __rte_unused)
+{
+	uint8_t rx_adapter_id;
+
+	if (params == NULL || strlen(params) == 0 || ~isdigit(*params))
+		return -1;
+
+	/* Get Rx adapter ID from parameter string */
+	rx_adapter_id = atoi(params);
+	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
+
+	/* Reset Rx adapter stats */
+	if (rte_event_eth_rx_adapter_stats_reset(rx_adapter_id)) {
+		RTE_EDEV_LOG_ERR("Failed to reset Rx adapter stats\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+static int
+handle_rxa_get_queue_conf(const char *cmd __rte_unused,
+			  const char *params,
+			  struct rte_tel_data *d)
+{
+	uint8_t rx_adapter_id;
+	uint16_t rx_queue_id;
+	int eth_dev_id;
+	char *token, *l_params;
+	struct rte_event_eth_rx_adapter_queue_conf queue_conf;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get Rx adapter ID from parameter string */
+	l_params = strdup(params);
+	token = strtok(l_params, ",");
+	rx_adapter_id = strtoul(token, NULL, 10);
+	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(rx_adapter_id, -EINVAL);
+
+	token = strtok(NULL, ",");
+	if (token == NULL || strlen(token) == 0 || !isdigit(*token))
+		return -1;
+
+	/* Get device ID from parameter string */
+	eth_dev_id = strtoul(token, NULL, 10);
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(eth_dev_id, -EINVAL);
+
+	token = strtok(NULL, ",");
+	if (token == NULL || strlen(token) == 0 || !isdigit(*token))
+		return -1;
+
+	/* Get Rx queue ID from parameter string */
+	rx_queue_id = strtoul(token, NULL, 10);
+	if (rx_queue_id >= rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
+		RTE_EDEV_LOG_ERR("Invalid rx queue_id %u", rx_queue_id);
+		return -EINVAL;
+	}
+
+	token = strtok(NULL, "\0");
+	if (token != NULL)
+		RTE_EDEV_LOG_ERR("Extra parameters passed to eventdev"
+				 " telemetry command, igrnoring");
+
+	if (rte_event_eth_rx_adapter_queue_conf_get(rx_adapter_id, eth_dev_id,
+						    rx_queue_id, &queue_conf)) {
+		RTE_EDEV_LOG_ERR("Failed to get Rx adapter queue config");
+		return -1;
+	}
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_u64(d, "rx_adapter_id", rx_adapter_id);
+	rte_tel_data_add_dict_u64(d, "eth_dev_id", eth_dev_id);
+	rte_tel_data_add_dict_u64(d, "rx_queue_id", rx_queue_id);
+	RXA_ADD_DICT(queue_conf, rx_queue_flags);
+	RXA_ADD_DICT(queue_conf, servicing_weight);
+	RXA_ADD_DICT(queue_conf.ev, queue_id);
+	RXA_ADD_DICT(queue_conf.ev, sched_type);
+	RXA_ADD_DICT(queue_conf.ev, priority);
+	RXA_ADD_DICT(queue_conf.ev, flow_id);
+
+	return 0;
+}
+
+RTE_INIT(rxa_init_telemetry)
+{
+	rte_telemetry_register_cmd("/eventdev/rxa_stats",
+		handle_rxa_stats,
+		"Returns Rx adapter stats. Parameter: rx_adapter_id");
+
+	rte_telemetry_register_cmd("/eventdev/rxa_stats_reset",
+		handle_rxa_stats_reset,
+		"Reset Rx adapter stats. Parameter: rx_adapter_id");
+
+	rte_telemetry_register_cmd("/eventdev/rxa_queue_conf",
+		handle_rxa_get_queue_conf,
+		"Returns Rx queue config. Parameter: rxa_id, DevID, que_id");
+}
diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h b/lib/eventdev/rte_event_eth_rx_adapter.h
index 70ca427..acabed4 100644
--- a/lib/eventdev/rte_event_eth_rx_adapter.h
+++ b/lib/eventdev/rte_event_eth_rx_adapter.h
@@ -216,6 +216,10 @@  struct rte_event_eth_rx_adapter_stats {
 	/**< Eventdev enqueue count */
 	uint64_t rx_enq_retry;
 	/**< Eventdev enqueue retry count */
+	uint64_t rx_event_buf_count;
+	/**< Rx event buffered count */
+	uint64_t rx_event_buf_size;
+	/**< Rx event buffer size */
 	uint64_t rx_dropped;
 	/**< Received packet dropped count */
 	uint64_t rx_enq_start_ts;