Re: [PATCH v5 06/20] event/sw: add support for event queues

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

On Mon, Mar 27, 2017 at 03:17:48PM +0000, Van Haaren, Harry wrote:
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Monday, March 27, 2017 8:45 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [PATCH v5 06/20] event/sw: add support for event queues
> > 
> > On Fri, Mar 24, 2017 at 04:53:01PM +0000, Harry van Haaren wrote:
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > Add in the data structures for the event queues, and the eventdev
> > > functions to create and destroy those queues.
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > ---
> 
> <snip>
> 
> > > +static int32_t
> > > +qid_init(struct sw_evdev *sw, unsigned int idx, int type,
> > > +		const struct rte_event_queue_conf *queue_conf)
> > > +{
> > > +	unsigned int i;
> > > +	int dev_id = sw->data->dev_id;
> > > +	int socket_id = sw->data->socket_id;
> > > +	char buf[IQ_RING_NAMESIZE];
> > > +	struct sw_qid *qid = &sw->qids[idx];
> > > +
> > > +	for (i = 0; i < SW_IQS_MAX; i++) {
> > 
> > Just for my understanding, Are 4(SW_IQS_MAX) iq rings created to address
> > different priority for each enqueue operation? What is the significance of
> > 4(SW_IQS_MAX) here?
> 
> Yes each IQ represents a priority level. There is a compile-time define (SW_IQS_MAX) which allows setting the number of internal-queues at each queue stage. The default number of priorities is currently 4.

OK. The reason why I asked because, If i understood it correctly the
PRIO_TO_IQ is not normalizing it correctly if SW_IQS_MAX == 4.

I thought following mapping will be the correct normalization if SW_IQS_MAX
== 4

What do you think?

priority----iq
0 - 63    -> 0
64 -127   -> 1
127 -191  -> 2
192 - 255 -> 3

Snippet from header file:
uint8_t priority;
/**< Event priority relative to other events in the
 * event queue. The requested priority should in the
 * range of  [RTE_EVENT_DEV_PRIORITY_HIGHEST,
 * RTE_EVENT_DEV_PRIORITY_LOWEST].
 * The implementation shall normalize the requested
 * priority to supported priority value.
 * Valid when the device has
 * RTE_EVENT_DEV_CAP_EVENT_QOS capability.
 */

> 
> 
> > > +static int
> > > +sw_queue_setup(struct rte_eventdev *dev, uint8_t queue_id,
> > > +		const struct rte_event_queue_conf *conf)
> > > +{
> > > +	int type;
> > > +
> > > +	switch (conf->event_queue_cfg) {
> > > +	case RTE_EVENT_QUEUE_CFG_SINGLE_LINK:
> > > +		type = SW_SCHED_TYPE_DIRECT;
> > > +		break;
> > 
> > event_queue_cfg is a bitmap. It is valid to have
> > RTE_EVENT_QUEUE_CFG_SINGLE_LINK | RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY.
> > i.e An atomic schedule type queue and it has only one port linked to
> > dequeue the events.
> > So in the above context, The switch case is not correct. i.e
> > it goes to the default condition. Right?
> > Is this intentional?
> > 
> > If I understand it correctly, Based on the use case(grouped based event
> > pipelining), you have shared in
> > the documentation patch. RTE_EVENT_QUEUE_CFG_SINGLE_LINK used for last
> > stage(last queue). One option is if SW PMD cannot support
> > RTE_EVENT_QUEUE_CFG_SINGLE_LINK | RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY mode
> > then even tough application sets the RTE_EVENT_QUEUE_CFG_SINGLE_LINK |
> > RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY, driver can ignore
> > RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY. But I am not sure the case where
> > application sets RTE_EVENT_QUEUE_CFG_SINGLE_LINK in the middle of the pipeline.
> > 
> > Thoughts?
> 
> 
> I don't like the idea of the SW PMD ignoring flags for queues - the PMD has no idea if the queue is the final or middle of the pipeline as it's the applications usage which defines that.
> 
> 
> Does anybody have a need for a queue to be both Atomic *and* Single-link?  I understand the current API doesn't prohibit it, but I don't see the actual use-case in which that may be useful. Atomic implies load-balancing is occurring, single link implies there is only one consuming core. Those seem like opposites to me?
> 
> Unless anybody sees value in queue's having both, I suggest we update the documentation to specify that a queue is either load balanced, or single-link, and that setting both flags will result in -ENOTSUP being returned. (This check can be added to EventDev layer if consistent for all PMDs).

If I understand it correctly(Based on the previous discussions),
HW implementations(Cavium or NXP) does not
need to use RTE_EVENT_QUEUE_CFG_* flags for the operations(sched type
will be derived from event.sched_type on enqueue). So that means we are
free to tailor the header file based on the SW PMD requirement on this.
But semantically it has to be inline with rest of the header file.We can
work together to make it happen.

A few question on everyone benefit:

1) Does RTE_EVENT_QUEUE_CFG_SINGLE_LINK has any other meaning other than an
event queue linked only to single port?  Based on the discussions, It was
add in the header file so that SW PMD can know upfront only single port
will be linked to the given event queue. It is added as an optimization for SW
PMD. Does it has any functional expectation?


2) Based on following topology given in documentation patch for queue
based event pipelining,

  rx_port        w1_port
	 \     /         \
	  qid0 - w2_port - qid1
	       \         /     \
		 w3_port        tx_port

a) I understand, rx_port is feeding events to qid0
b) But, Do you see any issue with following model? IMO, It scales well
linearly based on number of cores available to work(Since it is ATOMIC to
ATOMIC). Nothing wrong with
qid1 just connects to tx_port, I am just trying understand the rational
behind it?

  rx_port        w1_port         w1_port
	 \     /         \     /
	  qid0 - w2_port - qid1- w2_port
	       \         /     \
		 w3_port         w3_port

3)
> Does anybody have a need for a queue to be both Atomic *and* Single-link?  I understand the current API doesn't prohibit it, but I don't see the actual use-case in which that may be useful. Atomic implies load-balancing is occurring, single link implies there is only one consuming core. Those seem like opposites to me?

I can think about the following use case:

topology:

  rx_port        w1_port
	 \     /         \
	  qid0 - w2_port - qid1
	       \         /     \
		 w3_port        tx_port

Use case:

Queue based event pipeling:
ORERDED(Stage1) to ATOMIC(Stage2) pipeline:
- For ingress order maintenance
- For executing Stage 1 in parallel for better scaling
i.e A fat flow can spray over N cores while maintaining the ingress
order when it sends out on the wire(after consuming from tx_port)

I am not sure how SW PMD work in the use case of ingress order maintenance.

But the HW and header file expects this form:
Snippet from header file:
--
 * The source flow ordering from an event queue is maintained when events are
 * enqueued to their destination queue within the same ordered flow context.
 *
 * Events from the source queue appear in their original order when dequeued
 * from a destination queue.
--
Here qid0 is source queue with ORDERED sched_type and qid1 is destination
queue with ATOMIC sched_type. qid1 can be linked to only port(tx_port).

Are we on same page? If not, let me know the differences? We will try to
accommodate the same in header file.

> 

> 
> 
> Counter-thoughts?


> 
> 
> > > +	case RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY:
> > > +		type = RTE_SCHED_TYPE_ATOMIC;
> > > +		break;
> > > +	case RTE_EVENT_QUEUE_CFG_ORDERED_ONLY:
> > > +		type = RTE_SCHED_TYPE_ORDERED;
> > > +		break;
> > > +	case RTE_EVENT_QUEUE_CFG_PARALLEL_ONLY:
> > > +		type = RTE_SCHED_TYPE_PARALLEL;
> > > +		break;
> > > +	case RTE_EVENT_QUEUE_CFG_ALL_TYPES:
> > > +		SW_LOG_ERR("QUEUE_CFG_ALL_TYPES not supported\n");
> > > +		return -ENOTSUP;
> > > +	default:
> > > +		SW_LOG_ERR("Unknown queue type %d requested\n",
> > > +			   conf->event_queue_cfg);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	struct sw_evdev *sw = sw_pmd_priv(dev);
> > > +	return qid_init(sw, queue_id, type, conf);
> > > +}