Problem with pktq_dequeue()'s interaction with pktq_barrier()

[Jason Thorpe <thorpej%me.com@localhost>]

pktq_barrier() exists to ensure that any packets enqueued in a pktqueue before the barrier is issued will be processed (or, at least, removed from the queue) before the barrier returns.

The way pktq_barrier() works is by inserting markers into the per-cpu queues, and then waiting for each of those markers to be seen by the consuming side.  Only one barrier can be pending on a pktqueue at any given time.  I’m skipping over a lot of the details here, but those don’t matter for the purposes of this discussion.

The typical pattern for the consumer side of pktqueue() is to be notified of new packets by a software interrupt, and to pull them off in a loop until there are no more left, like so:

	while ((m = pktq_dequeue(my_pktq)) != NULL) {
		// Do stuff with the packet.
	}

The problem is that pktq_dequeue() currently returns NULL when it encounters a barrier marker, even if there are packets in the queue after the barrier marker.  This causes the consumer to break out of its loop.  Those packets are now at risk of being stranded in the queue unless additional packets are enqueued (which would trigger another softint).

At best I think this violates the principle of least astonishment and is more likely to simply be a bug.

The following patch fixes it.  Comments?

pktq_dequeue(): Prevent packets from getting stuck beind barrier markers.
pktq_barrier() ensures that all packets enqueued before the barrier have
been dequeued before the barrier returns.  However, previously, pktq_dequeue()
would return NULL when a barrier marker was encountered.  If there were
packets queued up behind the marker and no additional softint were scheduled
for the pktqueue, those packets would end up stranded.  pktq_dequeue() now
continues to the next slot after the marker, ensuring that processing can
continue after the barrier has been signaled.

diff --git a/sys/net/pktqueue.c b/sys/net/pktqueue.c
--- a/sys/net/pktqueue.c
+++ b/sys/net/pktqueue.c
@@ -450,7 +450,16 @@
 	if (__predict_false(m == PKTQ_MARKER)) {
 		/* Note the marker entry. */
 		atomic_inc_uint(&pq->pq_barrier);
-		return NULL;
+
+		/* Get the next queue entry. */
+		m = pcq_get(pktq_pcq(pq, ci));
+
+		/*
+		 * There can only be one barrier operation pending
+		 * on a pktqueue at any given time, so we can assert
+		 * that the next item is not a marker.
+		 */
+		KASSERT(m != PKTQ_MARKER);
 	}
 	if (__predict_true(m != NULL)) {
 		pktq_inc_count(pq, PQCNT_DEQUEUE);

-- thorpej