Re: [dpdk-dev] [PATCH v1 1/5] eventdev: rx_adapter: add support to configure event buffer size

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

Hi Jerin,
  Please see the replies inline.

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, September 20, 2021 11:50 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 v1 1/5] eventdev: rx_adapter: add support
> to configure event buffer size
> 
> On Sat, Sep 18, 2021 at 6:41 PM Naga Harish K S V
> <s.v.naga.harish.k@intel.com> wrote:
> >
> > Currently Rx event buffer is static array with a default size of
> > 192(6*BATCH_SIZE).
> >
> > ``rte_event_eth_rx_adapter_create2`` api is added which takes ``struct
> > rte_event_eth_rx_adapter_params`` to configure event buffer size in
> > addition other params . The event buffer is allocated dynamically at
> > run time aligned to BATCH_SIZE + 2*BATCH_SIZE.
> >
> > Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > ---
> >
> > +/**
> > + * 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 */
> 
> See below.
> 
> > +};
> > +
> >  /**
> >   *
> >   * Callback function invoked by the SW adapter before it continues @@
> > -330,6 +339,40 @@ 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);
> >
> > +/**
> > + * Create a new ethernet Rx event adapter with the specified identifier.
> > + * This function allocates Rx adapter event buffer with the size
> > +specified
> > + * in rxa_params aligned to BATCH_SIZE plus (BATCH_SIZE+BATCH_SIZE)
> > +and
> > + * uses an internal configuration function that creates an event port.
> 
> This function may use for adding another
> rte_event_eth_rx_adapter_params:: value.
> So semantics of rte_event_eth_rx_adapter_params::event_buf_size you
> can document at in that structure.  This function,  you can tell it adapter
> creation varint with parameters or so See below.
> 

The documentation is updated as as per the review comments.

> > + * This default function reconfigures the event device with an
> > + * additional event port and setups up 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.
> > + *
> > + * @param id
> > + *  The identifier of the ethernet Rx event adapter.
> > + *
> > + * @param dev_id
> > + *  The identifier of the event device to configure.
> > + *
> > + * @param rxa_params
> > + *  Pointer to struct rte_event_eth_rx_adapter_params containing
> > + *  size to allocate rx event buffer.
> 
> Value NULL is allowed to represent the default values or so.
> 

The api is updated to treat NULL pointer for adapter params with default values.

> > + *
> > + * @param port_config
> > + *  Argument of type *rte_event_port_conf* that is passed to the
> > +conf_cb
> > + *  function.
> > + *
> > + * @return
> > + *   - 0: Success
> > + *   - <0: Error code on failure
> > + */
> > +__rte_experimental
> > +int rte_event_eth_rx_adapter_create2(uint8_t id, uint8_t dev_id,
> > +                       struct rte_event_eth_rx_adapter_params *rxa_params,
> > +                       struct rte_event_port_conf *port_config);
> 
> Couple of suggestion on API name and prototype:
> - I think, we can remove 2 version and give more meaningful,name like
> rte_event_eth_rx_adapter_create_with_param() or so
> 
> - Keep new parameter as last to have better compatibility i.e
> rte_event_eth_rx_adapter_create_with_param(uint8_t id, uint8_t dev_id,
> struct rte_event_port_conf *port_config, struct
> rte_event_eth_rx_adapter_params *rxa_params)

The function name and parameters are adjusted as suggested.

Regards
Harish