> +
> +/* 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
