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>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, January 25, 2023 4:08 PM
> 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 Wed, Jan 25, 2023 at 3:22 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: 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?
> 
> OK. As long as when case INTERNAL PORT it return -ENOSUP now.
> 
> >
> > > > 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?
> 
> Yes. No need recompile if ABI not breaking.
> 
> > 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?
> 
> The problematic case is:
> 
> Adapter implementation of 23.07(Assuming there is change params) field
> needs to work with application of 23.03.
> rte_event_eth_rx_adapter_runtime_params_init() will sove that.
> 

As rte_event_eth_rx_adapter_runtime_params_init() initializes only reserved fields to zero,  it may not solve the issue in this case.
The old application only tries to set/get previous valid fields and the newly used fields may still contain junk value.
If the application wants to make use of any the newly used params, the application changes are required anyway.

> >
> > > >
> > > > > > +};
> > > > > > +
> > > > > >  /**
> > > > > >   *
> > > > > >   * 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);
> > > > > >