Re: [dpdk-dev] [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Wed, 9 Aug 2017 02:23:15 +0000
> From: "Eads, Gage" <gage.eads@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "Rao, Nikhil" <nikhil.rao@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "thomas@monjalon.net" <thomas@monjalon.net>, "Richardson, Bruce"
>  <bruce.richardson@intel.com>, "Van Haaren, Harry"
>  <harry.van.haaren@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
>  "Vangati, Narender" <narender.vangati@intel.com>, "Gujjar, Abhinandan S"
>  <abhinandan.gujjar@intel.com>
> Subject: RE: [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues
> 
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Thursday, August 3, 2017 1:23 AM
> > To: Eads, Gage <gage.eads@intel.com>
> > Cc: Rao, Nikhil <nikhil.rao@intel.com>; dev@dpdk.org; thomas@monjalon.net;
> > Richardson, Bruce <bruce.richardson@intel.com>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com;
> > nipun.gupta@nxp.com; Vangati, Narender <narender.vangati@intel.com>;
> > Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> > Subject: Re: [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues
> > 
> > -----Original Message-----
> > > Date: Wed, 2 Aug 2017 19:19:32 +0000
> > > From: "Eads, Gage" <gage.eads@intel.com>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Rao, Nikhil"
> > >  <nikhil.rao@intel.com>
> > > CC: "dev@dpdk.org" <dev@dpdk.org>, "thomas@monjalon.net"
> > >  <thomas@monjalon.net>, "Richardson, Bruce"
> > > <bruce.richardson@intel.com>,  "Van Haaren, Harry"
> > <harry.van.haaren@intel.com>, "hemant.agrawal@nxp.com"
> > >  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com"
> > > <nipun.gupta@nxp.com>,  "Vangati, Narender"
> > <narender.vangati@intel.com>, "Gujjar, Abhinandan S"
> > >  <abhinandan.gujjar@intel.com>
> > > Subject: RE: [PATCH 1/2] eventdev: add event adapter for ethernet Rx
> > > queues
> > >
> > >
> > > <snip>
> > >
> > > > > >
> > > > > > 5) specifying rte_event_eth_rx_adapter_conf.rx_event_port_id on
> > > > > > rte_event_eth_rx_adapter_create() would waste one HW eventdev
> > > > > > port if its happen to be used RX_ADAPTER_CAP_INBUILT_PORT on
> > > > rte_event_eth_rx_adapter_queue_add().
> > > > > > unlike SW eventdev port, HW eventdev ports are costly so I
> > > > > > think, We need to have another eventdev PMD ops to create
> > service/producer ports.
> > > > > > Or any other scheme that creates
> > > > > > rte_event_eth_rx_adapter_conf.rx_event_port_id
> > > > > > on demand by common code.
> > > > > >
> > > > >
> > > > > One solution is:
> > > > >
> > > > > struct rte_event_eth_rx_adapter_conf {
> > > > >     uint8_t dev_id;
> > > > >
> > > > >     int (*conf_cb)(uint8_t id, uint8_t port_id, uint32_t flags,
> > > > > struct rte_event_eth_rx_adapter_conf *conf);
> > > > >
> > > > >     unsigned int max_nb_rx;
> > > > >
> > > > >     int event_port_id;
> > > > >
> > > > >     char service_name[];
> > > > > }
> > > > >
> > > > > Where dev_id and conf_cb have to be specified in the create call,
> > > > > but event_port_id and service_name will be filled in when
> > > > > conf_cb() is invoked
> > > >
> > > > I was thinking like event_port_id will be rte_event_port_count() + 1.
> > > > ie When adapter needs the additional port, It can
> > > > - stop the eventdev
> > > > - reconfigure with rte_event_queue_count() , rte_event_port_count()
> > > > + 1
> > > > - start the eventdev.
> > > >
> > > > The only problem with callback is that all the application needs to implement
> > it.
> > > > If you think, application need more control then we can expose
> > > > callback and if it is NULL then default handler can be called in common code.
> > > >
> > >
> > > I don't think we can rely on there being another port available -- a user may
> > have configured the sw eventdev with all 64 ports, for instance.
> > 
> > On that case, irrespective any scheme(callback vs non callback) the adapter
> > creation would fail. Right?
> > 
> > > What if the user is required to calculate cfg.nb_event_ports as a function of
> > the RX_ADAPTER_CAP_INBUILT_PORT capability (i.e. add a port if the capability
> > is not set), such that a reconfigure is not required?
> > 
> > We have only one NON INBUILT eventdev port per adapter. Right? i.e in the v1
> > spec it was rte_event_eth_rx_adapter_conf.event_port_id,
> > How about it can be rte_event_port_count() + 1 ? Since we are NOT linking this
> > port, the context call be kept in adapter itself. Right?
> 
> It could be. Thinking on it some more, I'm a little concerned about doing configuration without the application's knowledge. Possible issues that could arise:
> - The user later reconfigures the event device with fewer ports and the adapter's port becomes invalid, or reconfigures it with more ports and begins using the port the adapter is using
> - rte_event_port_count() + 1 extends beyond the PMD's capabilities (the sw PMD is hard-coded to support a max of 64 ports, for example)
> 
> Having the user be responsible for the port configuration could avoid these problems. Since the user needs to check the <eventdev, ethdev> pair's capabilities for the CAP_ADD_QUEUE anyway, they could also check for INBUILT_PORT and decide whether or not to request an additional port at eventdev configure time -- thereby ensuring they don't waste a port when using hardware with inbuilt ports. And this keeps the configuration code in one place (the app), rather than spread across the app, adapter, and potentially the conf_cb.

OK.Sounds reasonable.May be we can push the responsibility to application.We could have a
helper function using the proposed adapter API. That helper function
would create the adapter based on the capability for the _default_ case.
Applications free to use the raw adapter API to get more control if required.
Otherwise we will duplicate the code in all the applications.

> 
> Besides these concerns, I think the transparent configuration approach (plus conf_cb when necessary to override) would work, but could have issues in the aforementioned edge cases.
> 
> > >
> > > As for application control: that would be a useful option in the conf_cb
> > scheme. Some apps will want to configure the adapter's port (its
> > new_event_threshold, its queue depths) differently from the default.
> > 
> > struct rte_event_port_conf * can be passed on the adapter create if application
> > needs more control.
> > 
> > >
> > > Thanks,
> > > Gage