RE: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs

["Naga Harish K, S V" <s.v.naga.harish.k@intel.com>]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, January 25, 2023 9:42 AM
> To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Cc: jerinj@marvell.com; Carrillo, Erik G <erik.g.carrillo@intel.com>; Gujjar,
> Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> Jayatheerthan, Jay <jay.jayatheerthan@intel.com>
> Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs
> 
> On Tue, Jan 24, 2023 at 6:37 PM Naga Harish K, S V
> <s.v.naga.harish.k@intel.com> wrote:
> >
> > Hi Jerin,
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Tuesday, January 24, 2023 10:00 AM
> > > To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> > > Cc: jerinj@marvell.com; Carrillo, Erik G
> > > <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S
> > > <abhinandan.gujjar@intel.com>; dev@dpdk.org; Jayatheerthan, Jay
> > > <jay.jayatheerthan@intel.com>
> > > Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs
> > >
> > > On Mon, Jan 23, 2023 at 11:35 PM Naga Harish K S V
> > > <s.v.naga.harish.k@intel.com> wrote:
> > > >
> > > > The adapter configuration parameters defined in the ``struct
> > > > rte_event_eth_rx_adapter_runtime_params`` can be configured and
> > > > retrieved using ``rte_event_eth_rx_adapter_runtime_params_set``
> > > > and ``rte_event_eth_tx_adapter_runtime_params_get`` respectively.
> > > >
> > > > Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> > >
> > > > diff --git a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > > > b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > > > index 461eca566f..2207d6ffc3 100644
> > > > --- a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > > > +++ b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
> > > > @@ -185,6 +185,26 @@ flags for handling received packets, event
> > > > queue identifier, scheduler type,  event priority, polling
> > > > frequency of the receive queue and flow identifier  in struct
> > > ``rte_event_eth_rx_adapter_queue_conf``.
> > > >
> > > > +Set/Get adapter runtime configuration parameters
> > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > +
> > > > +The runtime configuration parameters of adapter can be set/read
> > > > +using ``rte_event_eth_rx_adapter_runtime_params_set()`` and
> > > > +``rte_event_eth_rx_adapter_runtime_params_get()`` respectively.
> > > > +The parameters that can be set/read are defined in ``struct
> > > rte_event_eth_rx_adapter_runtime_params``.
> > >
> > > Good.
> > >
> > > > +
> > > > +``rte_event_eth_rx_adapter_create()`` or
> > > > +``rte_event_eth_rx_adapter_create_with_params()`` configures the
> > > > +adapter with default value for maximum packets processed per
> > > > +request to
> > > 128.
> > > > +``rte_event_eth_rx_adapter_runtime_params_set()`` function allows
> > > > +to reconfigure maximum number of packets processed by adapter per
> > > > +service request. This is alternative to configuring the maximum
> > > > +packets processed per request by adapter by using
> > > > +``rte_event_eth_rx_adapter_create_ext()`` with parameter
> > > ``rte_event_eth_rx_adapter_conf::max_nb_rx``.
> > >
> > > This paragraph is not needed IMO. As it is specific to a driver, and
> > > we can keep Doxygen comment only.
> > >
> > >
> > > > +
> > > > +``rte_event_eth_rx_adapter_runtime_parmas_get()`` function
> > > > +retrieves the configuration parameters that are defined in
> > > > +``struct
> > > rte_event_eth_rx_adapter_runtime_params``.
> > >
> > > Good.
> > >
> > > > +
> > > >  Getting and resetting Adapter queue stats
> > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
> > > > b/lib/eventdev/rte_event_eth_rx_adapter.c
> > > > index 34aa87379e..d8f3e750b7 100644
> > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> > > > @@ -35,6 +35,8 @@
> > > >  #define MAX_VECTOR_NS          1E9
> > > >  #define MIN_VECTOR_NS          1E5
> > > >
> > > > +#define RXA_NB_RX_WORK_DEFAULT 128
> > > > +
> > > >  #define ETH_RX_ADAPTER_SERVICE_NAME_LEN        32
> > > >  #define ETH_RX_ADAPTER_MEM_NAME_LEN    32
> > > >
> > > > @@ -1554,7 +1556,7 @@ rxa_default_conf_cb(uint8_t id, uint8_t
> dev_id,
> > > >         }
> > > >
> > > >         conf->event_port_id = port_id;
> > > > -       conf->max_nb_rx = 128;
> > > > +       conf->max_nb_rx = RXA_NB_RX_WORK_DEFAULT;
> > > >         if (started)
> > > >                 ret = rte_event_dev_start(dev_id);
> > > >         rx_adapter->default_cb_arg = 1; @@ -3436,6 +3438,90 @@
> > > > rte_event_eth_rx_adapter_instance_get(uint16_t eth_dev_id,
> > > >         return -EINVAL;
> > > >  }
> > >
> > > > +
> > > > +int
> > > > +rte_event_eth_rx_adapter_runtime_params_set(uint8_t id,
> > > > +               struct rte_event_eth_rx_adapter_runtime_params
> > > > +*params) {
> > > > +       struct event_eth_rx_adapter *rxa;
> > > > +       int ret;
> > > > +
> > > > +       if (params == NULL)
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (rxa_memzone_lookup())
> > > > +               return -ENOMEM;
> > >
> > > Introduce an adapter callback and move SW adapter related logic
> > > under callback handler.
> > >
> > >
> > Do you mean introducing eventdev PMD callback for HW implementation?
> 
> Yes.
> 

Can this be taken care as and when the HW implementation is required?

> > There are no adapter callback currently for service based SW
> Implementation.
> >
> > > > +
> > > > +       rxa = rxa_id_to_adapter(id);
> > > > +       if (rxa == NULL)
> > > > +               return -EINVAL;
> > > > +
> > > > +       ret = rxa_caps_check(rxa);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       rte_spinlock_lock(&rxa->rx_lock);
> > > > +       rxa->max_nb_rx = params->max_nb_rx;
> > > > +       rte_spinlock_unlock(&rxa->rx_lock);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +rte_event_eth_rx_adapter_runtime_params_get(uint8_t id,
> > > > +               struct rte_event_eth_rx_adapter_runtime_params
> > > > +*params) {
> > > > +       struct event_eth_rx_adapter *rxa;
> > > > +       int ret;
> > > > +
> > > > +       if (params == NULL)
> > > > +               return -EINVAL;
> > >
> > >
> > > Introduce an adapter callback and move SW adapter related logic
> > > under callback handler.
> > >
> > >
> >
> > Same as above
> >
> > > > +
> > > > +       if (rxa_memzone_lookup())
> > > > +               return -ENOMEM;
> > >  +
> > > > +       rxa = rxa_id_to_adapter(id);
> > > > +       if (rxa == NULL)
> > > > +               return -EINVAL;
> > > > +
> > > > +       ret = rxa_caps_check(rxa);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       params->max_nb_rx = rxa->max_nb_rx;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/* RX-adapter telemetry callbacks */
> > > >  #define RXA_ADD_DICT(stats, s) rte_tel_data_add_dict_u64(d, #s,
> > > > stats.s)
> > > >
> > > >  static int
> > > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > index f4652f40e8..214ffd018c 100644
> > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > @@ -39,10 +39,21 @@
> > > >   *  - rte_event_eth_rx_adapter_queue_stats_reset()
> > > >   *  - rte_event_eth_rx_adapter_event_port_get()
> > > >   *  - rte_event_eth_rx_adapter_instance_get()
> > > > + *  - rte_event_eth_rx_adapter_runtime_params_get()
> > > > + *  - rte_event_eth_rx_adapter_runtime_params_set()
> > > >   *
> > > >   * The application creates an ethernet to event adapter using
> > > >   * rte_event_eth_rx_adapter_create_ext() or
> > > rte_event_eth_rx_adapter_create()
> > > >   * or rte_event_eth_rx_adapter_create_with_params() functions.
> > > > + *
> > > > + * rte_event_eth_rx_adapter_create() or
> > > > + rte_event_eth_adapter_create_with_params()
> > > > + * configures the adapter with default value of maximum packets
> > > > + processed per
> > > > + * iteration to RXA_NB_RX_WORK_DEFAULT(128).
> > > > + * rte_event_eth_rx_adapter_runtime_params_set() allows to
> > > > + re-configure maximum
> > > > + * packets processed per iteration. This is alternative to using
> > > > + * rte_event_eth_rx_adapter_create_ext() with parameter
> > > > + * rte_event_eth_rx_adapter_conf::max_nb_rx
> > >
> > > Move this to Doxygen comment against max_nb_rx
> > >
> > > > + *
> > > >   * 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 @@ -299,6 +310,19 @@ struct
> > > rte_event_eth_rx_adapter_params {
> > > >         /**< flag to indicate that event buffer is separate for
> > > > each queue */  };
> > > >
> > > > +/**
> > > > + * Adapter configuration parameters  */ struct
> > > > +rte_event_eth_rx_adapter_runtime_params {
> > > > +       uint32_t max_nb_rx;
> > > > +       /**< The adapter can return early if it has processed at least
> > > > +        * max_nb_rx mbufs. This isn't treated as a requirement; batching
> may
> > > > +        * cause the adapter to process more than max_nb_rx mbufs.
> > >
> > > Also tell it is valid only for INTERNAL PORT capablity is set.
> > >
> >
> > Do you mean, it is valid only for INTERNAL PORT capability is 'not' set?
> 
> Yes.
> 
> >
> > > > +        */
> > > > +       uint32_t rsvd[15];
> > > > +       /**< Reserved fields for future use */
> > >
> > > Introduce rte_event_eth_rx_adapter_runtime_params_init() to make
> > > sure rsvd is zero.
> > >
> >
> > The reserved fields are not used by the adapter or application. Not
> > sure Is it necessary to Introduce a new API to clear reserved fields.
> 
> When adapter starts using new fileds(when we add new fieds in future), the
> old applicaiton which is not using
> rte_event_eth_rx_adapter_runtime_params_init() may have junk value and
> then adapter implementation will behave bad.
> 
> 

does it mean, the application doesn't re-compile for the new DPDK?
When some of the reserved fields are used in the future, the application also may need to be recompiled along with DPDK right?
As the application also may need to use the newly consumed reserved fields?

> >
> > > > +};
> > > > +
> > > >  /**
> > > >   *
> > > >   * Callback function invoked by the SW adapter before it
> > > > continues @@
> > > > -377,7 +401,7 @@ int rte_event_eth_rx_adapter_create_ext(uint8_t
> > > > id,
> > > uint8_t dev_id,
> > > >   * Create a new ethernet Rx event adapter with the specified
> identifier.
> > > >   * This function uses an internal configuration function that creates an
> event
> > > >   * port. This default function reconfigures the event device with
> > > > an
> > > > - * additional event port and setups up the event port using the
> > > > port_config
> > > > + * additional event port and setup the event port using the
> > > > + port_config
> > > >   * parameter passed into this function. In case the application needs
> more
> > > >   * control in configuration of the service, it should use the
> > > >   * rte_event_eth_rx_adapter_create_ext() version.
> > > > @@ -743,6 +767,50 @@
> > > > rte_event_eth_rx_adapter_instance_get(uint16_t
> > > eth_dev_id,
> > > >                                       uint16_t rx_queue_id,
> > > >                                       uint8_t *rxa_inst_id);
> > > >