Re: [dpdk-dev] [PATCH v2 2/5] distributor: new packet distributor library

[Neil Horman <nhorman@tuxdriver.com>]

> +
> +/* flush the distributor, so that there are no outstanding packets in flight or
> + * queued up. */
Its not clear to me that this is a distributor only function.  You modified the
comments to indicate that lcores can't preform double duty as both a worker and
a distributor, which is fine, but it implies that there is a clear distinction
between functions that are 'worker' functions and 'distributor' functions.
While its for the most part clear-ish (workers call rte_distributor_get_pkt and
rte_distibutor_return_pkt, distibutors calls rte_distributor_create/process.
This is in a grey area.  the analogy I'm thinking of here are kernel workqueues.
Theres a specific workqueue thread that processes the workqueue, but any process
can sync or flush the workqueue, leading me to think this process can be called
by a worker lcore.

> +int
> +rte_distributor_flush(struct rte_distributor *d)
> +{
> +	unsigned wkr, total_outstanding = 0;
> +	unsigned flushed = 0;
> +	unsigned ret_start = d->returns.start,
> +			ret_count = d->returns.count;
> +
> +	for (wkr = 0; wkr < d->num_workers; wkr++)
> +		total_outstanding += d->backlog[wkr].count +
> +				!!(d->in_flight_tags[wkr]);
> +
> +	wkr = 0;
> +	while (flushed < total_outstanding) {
> +
> +		if (d->in_flight_tags[wkr] != 0 || d->backlog[wkr].count) {
> +			const int64_t data = d->bufs[wkr].bufptr64;
> +			uintptr_t oldbuf = 0;
> +
> +			if (data & RTE_DISTRIB_GET_BUF) {
> +				flushed += (d->in_flight_tags[wkr] != 0);
> +				if (d->backlog[wkr].count) {
> +					d->bufs[wkr].bufptr64 =
> +						backlog_pop(&d->backlog[wkr]);
> +					/* we need to mark something as being
> +					 * in-flight, but it doesn't matter what
> +					 * as we never check it except
> +					 * to check for non-zero.
> +					 */
> +					d->in_flight_tags[wkr] = 1;
> +				} else {
> +					d->bufs[wkr].bufptr64 =
> +							RTE_DISTRIB_GET_BUF;
> +					d->in_flight_tags[wkr] = 0;
> +				}
> +				oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
> +			} else if (data & RTE_DISTRIB_RETURN_BUF) {
> +				if (d->backlog[wkr].count == 0 ||
> +						move_backlog(d, wkr) == 0) {
> +					/* only if we move backlog,
> +					 * process this packet */
> +					d->bufs[wkr].bufptr64 = 0;
> +					oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
> +					flushed++;
> +					d->in_flight_tags[wkr] = 0;
> +				}
> +			}
> +
> +			store_return(oldbuf, d, &ret_start, &ret_count);
> +		}
> +
I know the comments for move_backlog say you use that function here rather than
what you do in distributor_process because you're tracking the flush count here.
That said, if you instead recomputed the total_outstanding count on each loop
iteration, and tested it for 0, I think you could just reduce the flush
operation to a looping call to rte_distributor_process.  It would save you
having to maintain the flush code and the move_backlog code separately, which
would be a nice savings.

> +		if (++wkr == d->num_workers)
> +			wkr = 0;
Nit: wkr = ++wkr % d->num_workers avoids the additional branch in your loop


Regards
Neil