Re: [dpdk-dev] [PATCH v3 1/3] eventdev: add rx queue info get api

[Jerin Jacob <jerinjacobk@gmail.com>]

()

On Wed, Sep 8, 2021 at 1:51 PM Kundapura, Ganapati
<ganapati.kundapura@intel.com> wrote:
>
> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: 07 September 2021 15:07
> > To: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> > Cc: Jayatheerthan, Jay <jay.jayatheerthan@intel.com>; dpdk-dev
> > <dev@dpdk.org>; Pavan Nikhilesh <pbhagavatula@marvell.com>
> > Subject: Re: [PATCH v3 1/3] eventdev: add rx queue info get api
> >
> > On Tue, Sep 7, 2021 at 2:20 PM Kundapura, Ganapati
> > <ganapati.kundapura@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: 07 September 2021 13:42
> > > > To: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> > > > Cc: Jayatheerthan, Jay <jay.jayatheerthan@intel.com>; dpdk-dev
> > > > <dev@dpdk.org>; Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > Subject: Re: [PATCH v3 1/3] eventdev: add rx queue info get api
> > > >
> > > >  in
> > > >
> > > > On Tue, Sep 7, 2021 at 12:15 PM Ganapati Kundapura
> > > > <ganapati.kundapura@intel.com> wrote:
> > > > >
> > > > > Added rte_event_eth_rx_adapter_queue_info_get() API to get rx
> > > > > queue information - event queue identifier, flags for handling
> > > > > received packets, schedular type, event priority, polling
> > > > > frequency of the receive queue and flow identifier in
> > > > > rte_event_eth_rx_adapter_queue_info structure
> > > > >
> > > > > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > > > >
> > > > > ---
> > > > > v3:
> > > > > * Split single patch into implementaion, test and document updation
> > > > >   patches separately
> > > >
> > > > > +struct rte_event_eth_rx_adapter_queue_info;
> > > > > +
> > > > > +/**
> > > > > + * Retrieve information about Rx queue. This callback is invoked
> > > > > +if
> > > > > + * the caps returned from the eventdev_eth_rx_adapter_caps_get(,
> > > > > +eth_port_id)
> > > > > + * has RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT set.
> > > >
> > > > It will useful for !RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT
> > case
> > > > too.
> > > >
> >
> >
> >
> > Missed this comment in v4
> Sorry I missed these comments
>
> rte_event_eth_rx_adapter_queue_info_get() calls PMD callback if internal port cap is set,
> otherwise it implements to return the queue_info.
> PMD callback is for internal port and queue_info_get() api implements for non internal port

This API will be useful for !RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT case
too. In order to use this in a generic way, I suggest in following way,

In the rte_event_eth_rx_adapter_queue_add()  stores this config in
common code(i.e
file: ./lib/eventdev/rte_event_eth_rx_adapter.c and
rte_event_eth_rx_adapter_queue_add() function)
as named memzone or so.

Later when rte_event_eth_rx_adapter_queue_conf_get() called, the
config can be retrived
through named memzone lookup and have PMD specific callback like your patch
for overriding any parameter as neeed. i.e If PMD callback is NULL,
Application should
get the config struct as provided in rte_event_eth_rx_adapter_queue_add() with
rte_event_eth_rx_adapter_queue_conf_get().



>
> > > > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > index 182dd2e..75c0010 100644
> > > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h
> > > > > @@ -33,6 +33,7 @@
> > > > >   *  - rte_event_eth_rx_adapter_stop()
> > > > >   *  - rte_event_eth_rx_adapter_stats_get()
> > > > >   *  - rte_event_eth_rx_adapter_stats_reset()
> > > > > + *  - rte_event_eth_rx_adapter_queue_info_get()
> > > > >   *
> > > > >   * The application creates an ethernet to event adapter using
> > > > >   * rte_event_eth_rx_adapter_create_ext() or
> > > > > rte_event_eth_rx_adapter_create() @@ -140,6 +141,56 @@ typedef
> > int
> > > > (*rte_event_eth_rx_adapter_conf_cb) (uint8_t id, uint8_t dev_id,
> > > > >                         void *arg);
> > > > >
> > > > >  /**
> > > > > + * Rx queue info
> > > > > + */
> > > > > +struct rte_event_eth_rx_adapter_queue_info {
> > > >
> > > > Can we avoid the duplication of this structure and use
> > > > rte_event_eth_rx_adapter_queue_conf instead.
> > > >
> Agree
> > > > API can be rte_event_eth_rx_adapter_queue_conf_get() to align the
> > > > structure.
> Agree
> > > >
> > > > Also instead of every driver duplicating this code, How about
> > > > - common code stores the config in
> > > > rte_event_eth_rx_adapter_queue_add()
> > > > - common code stores the config in
> > > > rte_event_eth_rx_adapter_queue_conf_get()
>
> queue_add() stores the config in dev_info and queue_conf_get() retrieves the config from dev_info.
> Please clarify on common code to store and retrieve queue conf?


See above.


> > > > - Addtional PMD level API can be given incase, something needs to
> > > > overridden by Adapter.
> >
> Existing PMD callbacks like queue_add, queue_del, adapter_start, adapter_stop etc
> doesn't have any additional PMD level api.
> queue_info_get PMD callback is also similar.


See above.



>
> >
> > Missed addressing this comment in v4.