[dpdk-dev] [PATCH 16/22] event/dlb2: add dequeue and its burst variants

[Eads, Gage]

> -----Original Message-----
> From: McDaniel, Timothy <timothy.mcdaniel at intel.com>
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev at dpdk.org; Carrillo, Erik G <erik.g.carrillo at intel.com>; Eads, Gage
> <gage.eads at intel.com>; Van Haaren, Harry <harry.van.haaren at intel.com>;
> jerinj at marvell.com
> Subject: [PATCH 16/22] event/dlb2: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...

Please elaborate -- this commit message doesn't mention (e.g.) the use of
umonitor/umwait or the "sparse" dequeue function variants. I think the
average reviewer needs some more context in order to understand this change.

[...]

> +
> +static inline int
> +dlb2_process_dequeue_qes(struct dlb2_eventdev_port *ev_port,
> +			 struct dlb2_port *qm_port,
> +			 struct rte_event *events,
> +			 struct dlb2_dequeue_qe *qes,
> +			 int cnt)
> +{
> +	uint8_t *qid_mappings = qm_port->qid_mappings;
> +	int i, num, evq_id;
> +
> +	RTE_SET_USED(ev_port);  /* avoids unused variable error if stats off */

Looks like ev_port is used unconditionally later on: "evq_id = ev_port->link[0].queue_id;"

> +
> +	for (i = 0, num = 0; i < cnt; i++) {
> +		struct dlb2_dequeue_qe *qe = &qes[i];
> +		int sched_type_map[DLB2_NUM_HW_SCHED_TYPES] = {
> +			[DLB2_SCHED_ATOMIC] = RTE_SCHED_TYPE_ATOMIC,
> +			[DLB2_SCHED_UNORDERED] =
> RTE_SCHED_TYPE_PARALLEL,
> +			[DLB2_SCHED_ORDERED] = RTE_SCHED_TYPE_ORDERED,
> +			[DLB2_SCHED_DIRECTED] = RTE_SCHED_TYPE_ATOMIC,
> +		};
> +
> +		/* Fill in event information.
> +		 * Note that flow_id must be embedded in the data by
> +		 * the app, such as the mbuf RSS hash field if the data
> +		 * buffer is a mbuf.
> +		 */
> +		if (unlikely(qe->error)) {
> +			DLB2_LOG_ERR("QE error bit ON\n");
> +			DLB2_INC_STAT(ev_port->stats.traffic.rx_drop, 1);
> +			dlb2_consume_qe_immediate(qm_port, 1);
> +			continue; /* Ignore */
> +		}
> +
> +		events[num].u64 = qe->data;
> +		events[num].flow_id = qe->flow_id;
> +		events[num].priority = DLB2_TO_EV_PRIO((uint8_t)qe->priority);
> +		events[num].event_type = qe->u.event_type.major;
> +		events[num].sub_event_type = qe->u.event_type.sub;
> +		events[num].sched_type = sched_type_map[qe->sched_type];
> +		events[num].impl_opaque = qe->qid_depth;
> +
> +		/* qid not preserved for directed queues */
> +		if (qm_port->is_directed)
> +			evq_id = ev_port->link[0].queue_id;
> +		else
> +			evq_id = qid_mappings[qe->qid];
> +
> +		events[num].queue_id = evq_id;
> +		DLB2_INC_STAT(
> +			ev_port->stats.queue[evq_id].qid_depth[qe-
> >qid_depth],
> +			1);
> +		DLB2_INC_STAT(ev_port->stats.rx_sched_cnt[qe->sched_type],
> 1);
> +		DLB2_INC_STAT(ev_port->stats.traffic.rx_ok, 1);

Move this outside the loop and increment by 'num' rather than '1'?

> +		num++;
> +	}
> +
> +	return num;
> +}
> +
> +static inline int
> +dlb2_process_dequeue_four_qes(struct dlb2_eventdev_port *ev_port,
> +			      struct dlb2_port *qm_port,
> +			      struct rte_event *events,
> +			      struct dlb2_dequeue_qe *qes)
> +{
> +	int sched_type_map[] = {
> +		[DLB2_SCHED_ATOMIC] = RTE_SCHED_TYPE_ATOMIC,
> +		[DLB2_SCHED_UNORDERED] = RTE_SCHED_TYPE_PARALLEL,
> +		[DLB2_SCHED_ORDERED] = RTE_SCHED_TYPE_ORDERED,
> +		[DLB2_SCHED_DIRECTED] = RTE_SCHED_TYPE_ATOMIC,
> +	};
> +	const int num_events = DLB2_NUM_QES_PER_CACHE_LINE;
> +	uint8_t *qid_mappings = qm_port->qid_mappings;
> +	__m128i sse_evt[2];
> +
> +	RTE_SET_USED(ev_port);  /* avoids unused variable error, if stats off */

ev_port gets passed to dlb2_process_dequeue_qes, I don't think this line is necessary.

> +
> +	/* In the unlikely case that any of the QE error bits are set, process
> +	 * them one at a time.
> +	 */
> +	if (unlikely(qes[0].error || qes[1].error ||
> +		     qes[2].error || qes[3].error))
> +		return dlb2_process_dequeue_qes(ev_port, qm_port, events,
> +						 qes, num_events);

Thanks,
Gage