Re: [dpdk-dev] [PATCH v3 1/5] eventdev/rx_adapter: add event buffer size configurability

["Jayatheerthan, Jay" <jay.jayatheerthan@intel.com>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, September 29, 2021 10:46 AM
> To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Cc: Jerin Jacob <jerinj@marvell.com>; Jayatheerthan, Jay <jay.jayatheerthan@intel.com>; dpdk-dev <dev@dpdk.org>; Kundapura,
> Ganapati <ganapati.kundapura@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/5] eventdev/rx_adapter: add event buffer size configurability
> 
> On Wed, Sep 22, 2021 at 8:44 PM Naga Harish K S V
> <s.v.naga.harish.k@intel.com> wrote:
> >
> > Currently event buffer is static array with a default size defined
> > internally.
> >
> > To configure event buffer size from application,
> > ``rte_event_eth_rx_adapter_create_with_params`` api is added which
> > takes ``struct rte_event_eth_rx_adapter_params`` to configure event
> > buffer size in addition other params . The event buffer size is
> > rounded up for better buffer utilization and performance . In case
> > of NULL params argument, default event buffer size is used.
> >
> > Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> 
> 
> Changes look good to me.
> 
> @Jayatheerthan, Jay  Could review and Ack it?

@Jerin, sure. Will be able to get to it next week.

> 
> 
> >
> > ---
> > v3:
> > * updated documentation and code comments as per review comments.
> > * updated new create api test case name with suitable one.
> >
> > v2:
> > * Updated header file and rx adapter documentation as per review comments.
> > * new api name is modified as rte_event_eth_rx_adapter_create_with_params
> >   as per review comments.
> > * rxa_params pointer argument Value NULL is allowed to represent the
> >   default values
> >
> > v1:
> > * Initial implementation with documentation and unit tests.
> > ---
> >  .../prog_guide/event_ethernet_rx_adapter.rst  |  7 ++
> >  lib/eventdev/rte_event_eth_rx_adapter.c       | 98 +++++++++++++++++--
> >  lib/eventdev/rte_event_eth_rx_adapter.h       | 41 +++++++-
> >  lib/eventdev/version.map                      |  2 +
> >  4 files changed, 140 insertions(+), 8 deletions(-)
> >
> > diff --git a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > index 0780b6f711..dd753613bd 100644
> > --- a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > +++ b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > @@ -62,6 +62,13 @@ service function and needs to create an event port for it. The callback is
> >  expected to fill the ``struct rte_event_eth_rx_adapter_conf structure``
> >  passed to it.
> >
> > +If the application desires to control the event buffer size, it can use the
> > +``rte_event_eth_rx_adapter_create_with_params()`` api. The event buffer size is
> > +specified using ``struct rte_event_eth_rx_adapter_params::event_buf_size``.
> > +The function is passed the event device to be associated with the adapter
> > +and port configuration for the adapter to setup an event port if the
> > +adapter needs to use a service function.
> > +
> >  Adding Rx Queues to the Adapter Instance
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c b/lib/eventdev/rte_event_eth_rx_adapter.c
> > index f2dc69503d..7dec9a8734 100644
> > --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> > +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> > @@ -82,7 +82,9 @@ struct rte_eth_event_enqueue_buffer {
> >         /* Count of events in this buffer */
> >         uint16_t count;
> >         /* Array of events in this buffer */
> > -       struct rte_event events[ETH_EVENT_BUFFER_SIZE];
> > +       struct rte_event *events;
> > +       /* size of event buffer */
> > +       uint16_t events_size;
> >         /* Event enqueue happens from head */
> >         uint16_t head;
> >         /* New packets from rte_eth_rx_burst is enqued from tail */
> > @@ -919,7 +921,7 @@ rxa_buffer_mbufs(struct rte_event_eth_rx_adapter *rx_adapter,
> >                 dropped = 0;
> >                 nb_cb = dev_info->cb_fn(eth_dev_id, rx_queue_id,
> >                                        buf->last |
> > -                                      (RTE_DIM(buf->events) & ~buf->last_mask),
> > +                                      (buf->events_size & ~buf->last_mask),
> >                                        buf->count >= BATCH_SIZE ?
> >                                                 buf->count - BATCH_SIZE : 0,
> >                                        &buf->events[buf->tail],
> > @@ -945,7 +947,7 @@ rxa_pkt_buf_available(struct rte_eth_event_enqueue_buffer *buf)
> >         uint32_t nb_req = buf->tail + BATCH_SIZE;
> >
> >         if (!buf->last) {
> > -               if (nb_req <= RTE_DIM(buf->events))
> > +               if (nb_req <= buf->events_size)
> >                         return true;
> >
> >                 if (buf->head >= BATCH_SIZE) {
> > @@ -2164,12 +2166,15 @@ rxa_ctrl(uint8_t id, int start)
> >         return 0;
> >  }
> >
> > -int
> > -rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> > -                               rte_event_eth_rx_adapter_conf_cb conf_cb,
> > -                               void *conf_arg)
> > +static int
> > +rxa_create(uint8_t id, uint8_t dev_id,
> > +          struct rte_event_eth_rx_adapter_params *rxa_params,
> > +          rte_event_eth_rx_adapter_conf_cb conf_cb,
> > +          void *conf_arg)
> >  {
> >         struct rte_event_eth_rx_adapter *rx_adapter;
> > +       struct rte_eth_event_enqueue_buffer *buf;
> > +       struct rte_event *events;
> >         int ret;
> >         int socket_id;
> >         uint16_t i;
> > @@ -2184,6 +2189,7 @@ rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> >
> >         RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
> >         RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
> > +
> >         if (conf_cb == NULL)
> >                 return -EINVAL;
> >
> > @@ -2231,11 +2237,30 @@ rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> >                 rte_free(rx_adapter);
> >                 return -ENOMEM;
> >         }
> > +
> >         rte_spinlock_init(&rx_adapter->rx_lock);
> > +
> >         for (i = 0; i < RTE_MAX_ETHPORTS; i++)
> >                 rx_adapter->eth_devices[i].dev = &rte_eth_devices[i];
> >
> > +       /* Rx adapter event buffer allocation */
> > +       buf = &rx_adapter->event_enqueue_buffer;
> > +       buf->events_size = RTE_ALIGN(rxa_params->event_buf_size, BATCH_SIZE);
> > +
> > +       events = rte_zmalloc_socket(rx_adapter->mem_name,
> > +                                   buf->events_size * sizeof(*events),
> > +                                   0, socket_id);
> > +       if (events == NULL) {
> > +               RTE_EDEV_LOG_ERR("Failed to allocate mem for event buffer\n");
> > +               rte_free(rx_adapter->eth_devices);
> > +               rte_free(rx_adapter);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       rx_adapter->event_enqueue_buffer.events = events;
> > +
> >         event_eth_rx_adapter[id] = rx_adapter;
> > +
> >         if (conf_cb == rxa_default_conf_cb)
> >                 rx_adapter->default_cb_arg = 1;
> >         rte_eventdev_trace_eth_rx_adapter_create(id, dev_id, conf_cb,
> > @@ -2243,6 +2268,61 @@ rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> >         return 0;
> >  }
> >
> > +int
> > +rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> > +                               rte_event_eth_rx_adapter_conf_cb conf_cb,
> > +                               void *conf_arg)
> > +{
> > +       struct rte_event_eth_rx_adapter_params rxa_params;
> > +
> > +       /* use default values for adapter params */
> > +       rxa_params.event_buf_size = ETH_EVENT_BUFFER_SIZE;
> > +
> > +       return rxa_create(id, dev_id, &rxa_params, conf_cb, conf_arg);
> > +}
> > +
> > +int
> > +rte_event_eth_rx_adapter_create_with_params(uint8_t id, uint8_t dev_id,
> > +                       struct rte_event_port_conf *port_config,
> > +                       struct rte_event_eth_rx_adapter_params *rxa_params)
> > +{
> > +       struct rte_event_port_conf *pc;
> > +       int ret;
> > +       struct rte_event_eth_rx_adapter_params temp_params = {0};
> > +
> > +       if (port_config == NULL)
> > +               return -EINVAL;
> > +
> > +       /* use default values if rxa_parmas is NULL */
> > +       if (rxa_params == NULL) {
> > +               rxa_params = &temp_params;
> > +               rxa_params->event_buf_size = ETH_EVENT_BUFFER_SIZE;
> > +       }
> > +
> > +       if (rxa_params->event_buf_size == 0)
> > +               return -EINVAL;
> > +
> > +       pc = rte_malloc(NULL, sizeof(*pc), 0);
> > +       if (pc == NULL)
> > +               return -ENOMEM;
> > +
> > +       *pc = *port_config;
> > +
> > +       /* adjust event buff size with BATCH_SIZE used for fetching packets
> > +        * from NIC rx queues to get full buffer utilization and prevent
> > +        * unnecessary rollovers.
> > +        */
> > +       rxa_params->event_buf_size = RTE_ALIGN(rxa_params->event_buf_size,
> > +                                              BATCH_SIZE);
> > +       rxa_params->event_buf_size += BATCH_SIZE + BATCH_SIZE;
> > +
> > +       ret = rxa_create(id, dev_id, rxa_params, rxa_default_conf_cb, pc);
> > +       if (ret)
> > +               rte_free(pc);
> > +
> > +       return ret;
> > +}
> > +
> >  int
> >  rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> >                 struct rte_event_port_conf *port_config)
> > @@ -2252,12 +2332,14 @@ rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> >
> >         if (port_config == NULL)
> >                 return -EINVAL;
> > +
> >         RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
> >
> >         pc = rte_malloc(NULL, sizeof(*pc), 0);
> >         if (pc == NULL)
> >                 return -ENOMEM;
> >         *pc = *port_config;
> > +
> >         ret = rte_event_eth_rx_adapter_create_ext(id, dev_id,
> >                                         rxa_default_conf_cb,
> >                                         pc);
> > @@ -2286,6 +2368,7 @@ rte_event_eth_rx_adapter_free(uint8_t id)
> >         if (rx_adapter->default_cb_arg)
> >                 rte_free(rx_adapter->conf_arg);
> >         rte_free(rx_adapter->eth_devices);
> > +       rte_free(rx_adapter->event_enqueue_buffer.events);
> >         rte_free(rx_adapter);
> >         event_eth_rx_adapter[id] = NULL;
> >
> > @@ -2658,6 +2741,7 @@ rte_event_eth_rx_adapter_stats_get(uint8_t id,
> >
> >         stats->rx_packets += dev_stats_sum.rx_packets;
> >         stats->rx_enq_count += dev_stats_sum.rx_enq_count;
> > +
> >         return 0;
> >  }
> >
> > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h b/lib/eventdev/rte_event_eth_rx_adapter.h
> > index 3f8b362295..6e8b3085f8 100644
> > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > @@ -26,6 +26,7 @@
> >   * The ethernet Rx event adapter's functions are:
> >   *  - rte_event_eth_rx_adapter_create_ext()
> >   *  - rte_event_eth_rx_adapter_create()
> > + *  - rte_event_eth_rx_adapter_create_with_params()
> >   *  - rte_event_eth_rx_adapter_free()
> >   *  - rte_event_eth_rx_adapter_queue_add()
> >   *  - rte_event_eth_rx_adapter_queue_del()
> > @@ -36,7 +37,7 @@
> >   *
> >   * The application creates an ethernet to event adapter using
> >   * rte_event_eth_rx_adapter_create_ext() or rte_event_eth_rx_adapter_create()
> > - * functions.
> > + * or rte_event_eth_rx_adapter_create_with_params() functions.
> >   * The adapter needs to know which ethernet rx queues to poll for mbufs as well
> >   * as event device parameters such as the event queue identifier, event
> >   * priority and scheduling type that the adapter should use when constructing
> > @@ -256,6 +257,17 @@ struct rte_event_eth_rx_adapter_vector_limits {
> >          */
> >  };
> >
> > +/**
> > + * A structure to hold adapter config params
> > + */
> > +struct rte_event_eth_rx_adapter_params {
> > +       uint16_t event_buf_size;
> > +       /**< size of event buffer for the adapter.
> > +        * This value is rounded up for better buffer utilization
> > +        * and performance.
> > +        */
> > +};
> > +
> >  /**
> >   *
> >   * Callback function invoked by the SW adapter before it continues
> > @@ -356,6 +368,33 @@ int rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> >  int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> >                                 struct rte_event_port_conf *port_config);
> >
> > +/**
> > + * This is a variant of rte_event_eth_rx_adapter_create() with additional
> > + * adapter params specified in ``struct rte_event_eth_rx_adapter_params``.
> > + *
> > + * @param id
> > + *  The identifier of the ethernet Rx event adapter.
> > + *
> > + * @param dev_id
> > + *  The identifier of the event device to configure.
> > + *
> > + * @param port_config
> > + *  Argument of type *rte_event_port_conf* that is passed to the conf_cb
> > + *  function.
> > + *
> > + * @param rxa_params
> > + *  Pointer to struct rte_event_eth_rx_adapter_params.
> > + *  In case of NULL, default values are used.
> > + *
> > + * @return
> > + *   - 0: Success
> > + *   - <0: Error code on failure
> > + */
> > +__rte_experimental
> > +int rte_event_eth_rx_adapter_create_with_params(uint8_t id, uint8_t dev_id,
> > +                       struct rte_event_port_conf *port_config,
> > +                       struct rte_event_eth_rx_adapter_params *rxa_params);
> > +
> >  /**
> >   * Free an event adapter
> >   *
> > diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
> > index cd86d2d908..87586de879 100644
> > --- a/lib/eventdev/version.map
> > +++ b/lib/eventdev/version.map
> > @@ -138,6 +138,8 @@ EXPERIMENTAL {
> >         __rte_eventdev_trace_port_setup;
> >         # added in 20.11
> >         rte_event_pmd_pci_probe_named;
> > +       # added in 21.11
> > +       rte_event_eth_rx_adapter_create_with_params;
> >
> >         #added in 21.05
> >         rte_event_vector_pool_create;
> > --
> > 2.25.1
> >