Re: [dpdk-dev] [PATCH v2 1/6] eventdev: introduce event driven programming model

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

On Thu, Dec 08, 2016 at 09:57:52AM +0000, Bruce Richardson wrote:
> On Thu, Dec 08, 2016 at 07:18:01AM +0530, Jerin Jacob wrote:
> > On Wed, Dec 07, 2016 at 11:12:51AM +0000, Bruce Richardson wrote:
> > > On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> > > > + */
> > > > +int
> > > > +rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > > > +		    const struct rte_event_queue_link link[],
> > > > +		    uint16_t nb_links);
> > > > +
> > > 
> > > Hi again Jerin,
> > > 
> > > another small suggestion here. I'm not a big fan of using small
> > > structures to pass parameters into functions, especially when not all
> > > fields are always going to be used. Rather than use the event queue link
> > > structure, can we just pass in two array parameters here - the list of
> > > QIDs, and the list of priorities. In cases where the eventdev
> > > implementation does not support link prioritization, or where the app
> > > does not want different priority mappings , then the second
> > > array can be null [implying NORMAL priority for the don't care case].
> > > 
> > > 	int
> > > 	rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > > 		const uint8_t queues[], const uint8_t priorities[],
> > > 		uint16_t nb_queues);
> > > 
> > > This just makes mapping an array of queues easier, as we can just pass
> > > an array of ints directly in, and it especially makes it easier to
> > > create a single link via:
> > > 
> > >   rte_event_port_link(dev_id, port_id, &queue_id, NULL, 1);
> > 
> > The reason why I thought of creating "struct rte_event_queue_link",
> > - Its easy to add new parameter in link attributes if required
> 
> Make the priority value be in a struct, perhaps. That would allow for
> future expansion, while still making it easier for the case where people
> just want the mappings without any prioritization.

OK. I will change the API prototype to align with your proposal in v3

int
rte_event_port_link(uint8_t dev_id, uint8_t port_id,
	const uint8_t queues[], const uint8_t priorities[],
	uint16_t nb_links);

int
rte_event_port_links_get(uint8_t dev_id, uint8_t port_id,
	uint8_t queues[], uint8_t priorities[]);

> 
> > - Its _easy_ to implement PAUSE and RESUME in application
> > 
> > PAUSE:
> > nr_links = rte_event_port_links_get(,,link)
> > rte_event_port_unlink_all
> > 
> > RESUME:
> > rte_event_port_link(,,link, nr_links);
> 
> Ok, I had missed that implication. Since that is probably an important
> operation we might want to do, perhaps links_get API should be updated
> too to keep parameter matching.
> 
> /Bruce