Re-check for a VIO_DESC_READY data descriptor after short udelay()

Upon encountering the first !VIO_DESC_READY in vnet_walk_rx(), it is frequently worthwhile to re-check the descriptor status after a short microsecond delay, as a bursty sender could be actively populating the descriptors, and the short udelay() is far less expensive than rolling back to ldc_rx() and having to wake up and read data on another LDC message.

---
changes since v1: moved label again', variable retries' to this patchset.

 drivers/net/ethernet/sun/sunvnet.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

index 8f90b57..7b1f320 100644
@@ -390,11 +390,28 @@ static int vnet_walk_rx(struct vnet_port *port, struct vio_dring_state *dr,
 	viodbg(DATA, "vnet_walk_rx start[%08x] end[%08x]\n", start, end);
 
 	while (start != end) {
-		int ack = 0, err = vnet_walk_rx_one(port, dr, start, &ack);
+		int retries;
+		int ack = 0, err;
+
+		retries = 0;
+again:
+		err = vnet_walk_rx_one(port, dr, start, &ack);
 		if (err == -ECONNRESET)
 			return err;
-		if (err != 0)
-			break;
+		if (err != 0)  {
+			/* The descriptor was not READY. Retry with a
+			 * small delay, in case we have a bursty sender
+			 * that is actively populating the descriptors, to
+			 * reduce the overhead of stopping and re-entering
+			 * which would involve expensive LDC messages.
+			 */
+			if (retries++ < 3) {
+				udelay(4);
+				goto again;
+			} else {
+				break;
+			}
+		}
 		if (ack_start == -1)
 			ack_start = start;
 		ack_end = start;

1.8.4.2

Sowmini,
One of the things I found while looking at the code is that the
write memory barrier is in the wrong place. I have a patch to fix it, but it means the last descriptor will NOT be marked as ready until the cache is flushed, which may be delayed.
That fix may make this one moot. Maybe not, but certainly there is
an unnecessary delay in notifying the peer because of that bug. The fix is simply to move the "wmb()" after, instead of before, setting VIO_DESC_READY. I mentioned it in our stand-up last week, which you couldn't attend, because I pointed it out for the VDC driver, which also has the same problem.

You may want to retry with that change to see if the delay still
helps.

							+-DLS

I see.

I can certainly try that (could you please share that patch offline?) But this fudge-factored delay (which, fwiw, I too cringed to add) was actually motivated by some observations on other non-linux OS-es.

From: David L Stevens <[email protected]>
Date: Tue, 02 Sep 2014 12:27:19 -0400

The memory barrier exists in order to make sure the cookies et al. are globally visible before the VIO_DESC_READY. We don't want stores to be reordered such that the VIO_DESC_READY is seen too early.

I'm having a hard time imagining that putting the wmb() afterwards would help, except by adding a new delay in a different place.

I say that because the TX trigger is going to make a hypervisor call, and I bet that hypervisor trap syncs the store buffer of invoking cpu thread.

Thinking further, another issue to consider is that the wmb() might no be necessary considering all of the above, because the remote entity should not look at this descriptor until the tx trigger occurs.

Anyways, if the hypervisor trap to signal the tx trigger does not flush the local cpu thread's store buffer, then yes moving the memory barrier might make sense.

Ok, though David (dls) was just pointing out that a rmb() might be missing in vnet_walk_rx_one() before checking for READY descriptor

correct.

So, today the consumer already reads a series of descriptors based on a single LDC start trigger already in vnet_walk_rx() - it doesnt actually wait for an LDC "start" per descriptor.

Which is as it should be- the first "start" trigger of a burst is just an async signal from producer to consumer that data is ready for consumption.

A wmb() (and maybe a missing rmb()) should itself synchronize the critical state more efficiently than an ldc exchange. The last bit about VIO_DESC_READY may get flushed at some later point, and the delay in patch 2/2 slightly helps work around that fudge factor, but moving the wmb() before/after the VIO_DESC_READY (in my testing) doesn't really make a difference to correctness - the consumer first checks for VIO_DESC_READY, and by that time, the rest of the cookie info should have been correctly sync'ed by the memory barrier.

If we get the memory-barriers right,
the trigger-per-vnet_start_xmit is superfluous, and only serves to flood the ldc_channel (and make the env ripe for !tx_has_space_for()).

(And a side-effect of this is that avoids severl other udelay() loops that we'd have otherwise undergone as part of __vnet_tx_trigger(), which is also a healthy thing to avoid)

As such, it's expensive and unnecessary to cripple ourselves down to one LDC exchange per descriptor - the wmb() itself should ensure this correctly in a cheaper way.

BTW, even if you set the READY state before (not after) the wmb() there's still a small iperf-boost to be obtained by lingering around in vnet_walk_rx() (as in patch 2/2)- that boost is from avoiding what would otherwise be a ldc stop/start exchange between consumer and producer.

But I'm not too attached to this boost (aka patch 2/2)- benchmark-special code is always ugly.

--Sowmini

Stared at this a bit over the last two days, checked the documentation, discussed with dls offline - looks like
(a) the rmb() thing was mostly a red-herring/fud
(b) we do need the wmb()

The wmb() part is working correctly as designed:

The producer will do
/* code to set up cookies */
wmb(); /* makes sure above changes are committed */ d->hdr.state = VIO_DESC_READY;

the consumer will do

        if (desc->hdr.state != VIO_DESC_READY)
                return 1;
        err = vnet_rx_one(port, desc->size, desc->cookies, desc->ncookies);
                :
        desc->hdr.state = VIO_DESC_DONE;

So the vnet_rx_one() will only use valid cookie information at all times.

This allows the code to correctly able to read multiple READY descriptors for a single LDC trigger, which it already does today.
(and it would be needlessly inefficient to clamp this down to only one descriptor read per LDC-start in the vnet_rx())

So what (if any) is the outstanding question about wmb() at this point?

--Sowmini

Sorry for joining this thread late, please see my response below:

Yeah, we need the producer memory barrier before setting the descriptor state to ready. This ensures both data in the buffers and other fields in the descriptor are flushed before the state is visible to the consumer. I do not believe the HV trap caused by the trigger performs any sync of the store buffers.

Note, LDC message infrastructure is not a high performance infrastructure, relying on an ldc message for each packet can greatly impact performance. We need to reduce as many ldc messages as possible and rely on the ring more to pick up as many packets as possible without ldc messages.

The current implementation of vnet_start_xmit() performs all operations such as grabbing a descriptor, copying the data into the data buffers
and updating the descriptor with in the same lock. This limits the parallelism on the transmit path especially because the data copy operation is a bigger operation and blocks other transmitters more than it is needed. It is possible to split these operations where multiple transmitters can grab descriptors in sequence but perform the copy operation in parallel. When this accomplished, there will be more possibility of racing with descriptors getting ready quickly in a burst traffic situations. I believe the udelay() introduced in this patch will help even more when the transmitters are highly parallel.

-Raghuram

From: Sowmini Varadhan <[email protected]>
Date: Sat, 6 Sep 2014 17:02:53 -0400

I don't think there is any, thanks.

Sowmini,

I don't think this has anything to do with your patch, but my (new) concern with the
wmb() and no matching rmb() is this text from Documentation/memory-barrier.txt:

I'm no mb expert, and I know of no symptoms, but it appears to be saying that load reordering could result in a race where the READY flag could be set with old data in other descriptor fields due to loading them in a different order -- something it says wmb() on another CPU explicitly does not prevent.

The particular case would be adding to the ring at the same time the other side is removing from the ring, so no locks or LDC traffic would affect it.

So, it appears to me we have a missing rmb() that is needed and I don't know what leads you to believe it isn't.

							+-DLS

On (09/08/14 09:45), David L Stevens wrote:

First off, please don't drop email ids from the original email. I've re-added Raghuram Kothakota to fix that initial omission.

Second, this thread originally came up with your suggestion that we shold move the wmb() down one line. I looked at this, and consulted a few folks, all of whom agree that that is probably incorrect.

As Bob Picco pointed out to me:
" Placing the wmb() after probably won't have any negative consequences
BUT could. Suppose the compiler reorders the stores and the d->hdr.state store occurs before another part of the descriptor. We HV trap, and the trap doesn't flush the store buffer. The consumer could then see VIO_DESC_READY before the cookies arrray is updated. It would depend on how many cache lines are spanned by vio_net_desc and the store buffer organization."
thus I would be cautious about doing that.

Please see the explanation that I, and Raghuram offered.
The wmb() assures that the stores of the cookie state are ordered correctly by the consumer. The next store is for VIO_DESC_READY and the consumer will not proceed to read cookie state until this is visible. Thus no rmb() is needed.

On its side, the consumer resets the hdr.state to DONE after it consumes the cookies, and the DRING_STOPPLED announcement further informs the producer that the descriptor is available.

Hope that helps. If you think there is some sequence where this is insufficient, please explain with details.