e100 rx: or s and el bits

In commit d52df4a35af569071fda3f4eb08e47cc7023f094, the description talks about emulating another driver by setting addtional bits and the being unable to test when submitted. Seeing the & operator to set more bits made me suspicious, and indeed the bits are defined in positive logic:

	cb_s      = 0x4000,
	cb_el     = 0x8000,

So anding those together would be 0. I'm guessing they should
be or'd, but don't have hardware here to test, much like the committed patch. In fact, I'll let someone else do the compile test too. I'll update the comment.

Quoted 9 lines · Milton Miller

In commit d52df4a35af569071fda3f4eb08e47cc7023f094, the description talks about emulating another driver by setting addtional bits and the being unable to test when submitted. Seeing the & operator to set more bits made me suspicious, and indeed the bits are defined in positive logic:

    cb_s      = 0x4000,
    cb_el     = 0x8000,

Quoted 5 lines

So anding those together would be 0. I'm guessing they should
be or'd, but don't have hardware here to test, much like the committed patch. In fact, I'll let someone else do the compile test too. I'll update the comment.

I wonder if this worked for me because the hardware also spun on the link field being NULL? Since the RU base is also set to 0, the calculated physical address would be 0 as well. I would imagine if the hardware tried to read/write to very low addresses across PCI, there would be issues. I will retest with a small receive pool to try to hit the problem. I will also run these tests with the new patch and with a smaller receive pool (default is 256) to make the pool run out more often.

So far my testing has shown both the original and the new version of the S-bit patch work in that no corruption seemed to occur over long term runs.

Quoted 3 lines · David Acker

So far my testing has shown both the original and the new version of the S-bit patch work in that no corruption seemed to occur over long term runs.

I spoke too soon. Further testing has not gone well. If I use the default settings for CPU saver and drop the receive pool down to 16 buffers I can cause problems with various forms of the patch. With the original S-bit patch I can get:

The updated patch produced a different issue. We got an RNR interrupt indicating the receive unit got ahead of the software. The S-bit patch removed any handling of this issue as it assumed the hardware would spin
on the sbit. Apparently if both the S-bit and the EL-bit are set on the same RFD, it follows the EL-bit handling. Printing the stat/ack and status bytes on the RNR interrupts I get:

status
01001000 = 0x48 = RUS of 0010 = No Resources, CUS of 01 = Suspended

stat/ack
01010000 = 0x50 = FR, RNR
or
00010000 = 0x10 = RNR

Notice that the RUS went into No Resources and not suspended. Thus clearing the S-bit does not wake it up; it needs a new start command. I could not find documentation that states that the S-bit need only be cleared to take the RU out of suspended state. Before the S-bit patch the driver tried to track this need but that version of the driver didn't work for me either. By the way, I am using, "Intel 8255x 10/100 Mbps Ethernet Controller Family, Open Source Software Developer Manual, January 2006" as my documentation.

This got me looking at just how in the world this worked on the old eepro100 driver. It had another difference; it did not reap the last rx
buffer in the chain. It set a postponed bit and then picked it up on the next interrupt after more buffers had been allocated. It then noticed that the RU was in a suspended or no resources state and did a softreset.

I don't believe this avoid the last buffer trick really fixes the race. Imagine the following:
1. 4 buffers in receive pool, all freshly allocated
2. Hardware consumes 3 buffers
3. Software processes 3 buffers, begins to allocate new buffers
4. Hardware writes status bits into buffer 4 while software updates link and command word bits in buffer 4. They share a cache line and corrupt each other.

This appears to be possible with any of the versions of this driver I have seen. The problem is one of packet ownership. Once the driver gives a list of buffers to hardware, hardware owns them all. The driver can not safely change these buffers. Sadly, this means that the idea of the driver "staying ahead" of the hardware such that the hardware never runs out of resources will not work here. Once the driver gives the hardware a packet with S or EL bits set, it must let the hardware encounter the packet and return it to software.

I think the driver needs to protect the last entry in the ring by putting the S-bit on the entry before it. The first time the driver allocates a block of packets, it writes a new S-bit out on the next to last packet. As buffers complete it allocates more packets in the chain but does not set a new S-bit since the old one will stop hardware. It can not clear the old S-bit because the driver does not own the buffer, hardware does. After processing the s-bit packet the hardware will interrupt with a stat/ack of RNR and RUS of suspended. When software processes a packet with an old S-bit it allocates new buffers and sets the s-bit on the new next to last packet.

The above case changes now:
1. 4 buffers numbered 1-4 in a receive pool, all freshly allocated. S-bit is on buffer 3.
2. Hardware consumes 3 buffers, hits S-bit, RNR interrupts
3. Software processes 3 buffers, begins to allocate new buffers
4. Software sends resume once buffers are allocated, S-bit is on buffer 2.
5. Hardware gets resume. When it processed buffer 3, it saved the link to buffer 4 and thus resumes at buffer 4.

Here is a different flow where the software stays ahead:
1. 4 buffers numbered 1-4 in a receive pool, all freshly allocated. S-bit is on buffer 3.
2. Hardware consumes 2 buffers (1, 2).
3. Software processes buffers 1, 2, begins to allocate new buffers
4. Software buffers 1, 2 are allocated
5. Hardware consumes 1 buffer (#3) and hits S-bit, RNR interrupts.
6. Software consumes 1 buffer, (#3) and finds the old S-bit. It allocates a new buffer 3 and sets the S-bit on buffer 2.
7. Software sends resume, hardware continues at buffer 4.

In this setup, software will send a resume command every RING_SIZE packets. RNR interrupts will also occur every RING_SIZE packets. When hardware is faster than software, it will process RING_SIZE packets, RNR interrupt and wait for software to process all of them. When software is faster then hardware, hardware will still process RING_SIZE packets before interrupting but software will only need to allocate 1 packet or so before sending the resume so hardware will wait much less time.

This will probably slow things down since on a fast CPU, software will normally stay ahead of the hardware and the only PCI operations from the driver would be interrupt acks. With this change, we have PCI operations every 256 packets. I don't see how else to do this in a safe way on ARM (at least PXA255).

I am testing this over the weekend with a 16-buffer receive pool. If all goes well, I will send a patch early next week. It will basically back out the S-bit patch and then make the changes noted above.

While this will help the problem with the cache-incoherent DMA systems not running, it guarantees the hardware will stop every <ring-size> packets and wait for the cpu to respond to an interrupt. It would seem that this will lead to packet drops.

[download manual from site in source file]

In fact 6.4.3.4 says 82557 will start dropping frames immediately.

Looking at the descriptions around page 101:
(1) The link pointer, S, and EL is read when hw starts recieving the frame.
(2) Its pretty clear EL overrides S from the order of the descriptions in the text.

(3) 6.4.3.3.1 #4 looks intresting -- That is a RFD with size 0 skips frame fill and goes to the next packet.

How about putting a zero length descriptor in consistent memory to
suspend the rx unit before the last real frame? In other words fr0 ->
fr1 ... frN-2 -> frN-1 -> WaitHere0 -> FrN. We could then have 2 such
frames, and when we refill modify FrN to the new chain, with the WaitHere1 as its next-to-last, do the syncs, then clear the S bit on
WaitHere0. When the rx passes WaitHere0 we can reclaim it for the next
use (might want a slightly larger pool, basically need RxRingSize / RxRingFillBatch such frames.

In fact 6.4.3.4 says 82557 will start dropping frames immediately.

Looking at the descriptions around page 101:
(1) The link pointer, S, and EL is read when hw starts recieving the frame.
(2) Its pretty clear EL overrides S from the order of the descriptions in the text.
(3) 6.4.3.3.1 #4 looks intresting -- That is a RFD with size 0 skips frame fill and goes to the next packet.

How about putting a zero length descriptor in consistent memory to suspend the rx unit before the last real frame? In other words fr0 -> fr1 ... frN-2 -> frN-1 -> WaitHere0 -> FrN. We could then have 2 such frames, and when we refill modify FrN to the new chain, with the WaitHere1 as its next-to-last, do the syncs, then clear the S bit on WaitHere0. When the rx passes WaitHere0 we can reclaim it for the next use (might want a slightly larger pool, basically need RxRingSize / RxRingFillBatch such frames.

Can you resend against the latest kernel (2.6.22-rc1)?

And what does Intel think?

Quoted 3 lines · Jeff Garzik

Can you resend against the latest kernel (2.6.22-rc1)?

And what does Intel think?

I'm expecting at least a reply from Milton as the patch was sent to him. I haven't yet tested it but will certainly do so. At first glance it looks OK, and I'll try to put it under my colleague's noses who know e100 best.

A resend against 2.6.22-rc1 would be nice.

Quoted 3 lines · Auke

Quoted 2 lines · Jeff Garzik

Can you resend against the latest kernel (2.6.22-rc1)?

Quoted 6 lines

Quoted 1 line

And what does Intel think?

I'm expecting at least a reply from Milton as the patch was sent to him. I haven't yet tested it but will certainly do so. At first glance it looks OK, and I'll try to put it under my colleague's noses who know e100 best.

A resend against 2.6.22-rc1 would be nice.

Done. Below is a patch against 2.6.22-rc1. It combines removing the s-bit patch and applying the patch I previously sent.

Quoted 4 lines · David Acker

Quoted 3 lines · Auke

Quoted 2 lines · Jeff Garzik

Can you resend against the latest kernel (2.6.22-rc1)?

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

And what does Intel think?

I'm expecting at least a reply from Milton as the patch was sent to him. I haven't yet tested it but will certainly do so. At first glance it looks OK, and I'll try to put it under my colleague's noses who know e100 best.

A resend against 2.6.22-rc1 would be nice.

Done. Below is a patch against 2.6.22-rc1. It combines removing the s-bit patch and applying the patch I previously sent.

Oops. I missed one state in that patch. Since the el-bit buffer will normally not complete due to a zero size, we need to check if the buffer with no data has the el-bit set. Without this, you have to wait for the interrupt. Sorry about that...this was in the code I tested on my embedded system but got lost in the regular kernel patch.

On the ARM, their is a race condition between software allocating a new receive buffer and hardware writing into a buffer. The two race on touching the last Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal to 0. When hardware encounters this buffer it attempts to write data to it and then update Status Word bits and Actual Count in the RFD. At the same time software may try to clear the el-bit and set the link address to a new buffer. Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer in the chain. When the hardware encounters this buffer it stops and does not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers, it can update the tail of the list because it knows the hardware will stop at the buffer before it. Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer. If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and complete that one in error. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Signed-off-by David Acker <[email protected]>

---

drivers/net/e100.c renamed from drivers/net/e100.c.orig +117 -13

@@ -285,6 +285,12 @@ enum scb_status {
  	rus_mask         = 0x3C,
  };
 
+enum ru_state  {
+	RU_SUSPENDED = 0,
+	RU_RUNNING	 = 1,
+	RU_UNINITIALIZED = -1,
+};
+
  enum scb_stat_ack {
  	stat_ack_not_ours    = 0x00,
  	stat_ack_sw_gen      = 0x04,

@@ -526,6 +532,7 @@ struct nic {
  	struct rx *rx_to_use;
  	struct rx *rx_to_clean;
  	struct rfd blank_rfd;
+	enum ru_state ru_running;
 
  	spinlock_t cb_lock			____cacheline_aligned;
  	spinlock_t cmd_lock;

@@ -947,7 +954,7 @@ static void e100_get_defaults(struct nic
  		((nic->mac >= mac_82558_D101_A4) ? cb_cid : cb_i));
 
  	/* Template for a freshly allocated RFD */
-	nic->blank_rfd.command = cpu_to_le16(cb_el & cb_s);
+	nic->blank_rfd.command = cpu_to_le16(cb_el);
  	nic->blank_rfd.rbd = 0xFFFFFFFF;
  	nic->blank_rfd.size = cpu_to_le16(VLAN_ETH_FRAME_LEN);
 

@@ -1742,11 +1749,19 @@ static int e100_alloc_cbs(struct nic *ni
  	return 0;
  }
 
-static inline void e100_start_receiver(struct nic *nic)
+static inline void e100_start_receiver(struct nic *nic, struct rx *rx)
  {
-	/* Start if RFA is non-NULL */
-	if(nic->rx_to_clean->skb)
-		e100_exec_cmd(nic, ruc_start, nic->rx_to_clean->dma_addr);
+	if(!nic->rxs) return;
+	if(RU_SUSPENDED != nic->ru_running) return;
+
+	/* handle init time starts */
+	if(!rx) rx = nic->rxs;
+
+	/* (Re)start RU if suspended or idle and RFA is non-NULL */
+	if(rx->skb) {
+		e100_exec_cmd(nic, ruc_start, rx->dma_addr);
+		nic->ru_running = RU_RUNNING;
+	}
  }
 
  #define RFD_BUF_LEN (sizeof(struct rfd) + VLAN_ETH_FRAME_LEN)

@@ -1769,13 +1784,12 @@ static int e100_rx_alloc_skb(struct nic
  	}
 
  	/* Link the RFD to end of RFA by linking previous RFD to
-	 * this one, and clearing EL bit of previous.  */
+	 * this one.  We are safe to touch the previous RFD because
+	 * it is protected the before last buffer's el bit being set  */
  	if(rx->prev->skb) {
  		struct rfd *prev_rfd = (struct rfd *)rx->prev->skb->data;
  		put_unaligned(cpu_to_le32(rx->dma_addr),
  			(u32 *)&prev_rfd->link);
-		wmb();
-		prev_rfd->command &= ~cpu_to_le16(cb_el & cb_s);
  		pci_dma_sync_single_for_device(nic->pdev, rx->prev->dma_addr,
  			sizeof(struct rfd), PCI_DMA_TODEVICE);
  	}

@@ -1801,8 +1815,12 @@ static int e100_rx_indicate(struct nic *
  	DPRINTK(RX_STATUS, DEBUG, "status=0x%04X\n", rfd_status);
 
  	/* If data isn't ready, nothing to indicate */
-	if(unlikely(!(rfd_status & cb_complete)))
+	if(unlikely(!(rfd_status & cb_complete))) {
+		/* this allows for a fast restart without re-enabling interrupts */
+		if(le16_to_cpu(rfd->command) & cb_el)
+			nic->ru_running = RU_SUSPENDED;
  		return -ENODATA;
+	}
 
  	/* Get actual data size */
  	actual_size = le16_to_cpu(rfd->actual_size) & 0x3FFF;

@@ -1813,6 +1831,10 @@ static int e100_rx_indicate(struct nic *
  	pci_unmap_single(nic->pdev, rx->dma_addr,
  		RFD_BUF_LEN, PCI_DMA_FROMDEVICE);
 
+	/* this allows for a fast restart without re-enabling interrupts */
+	if(le16_to_cpu(rfd->command) & cb_el)
+		nic->ru_running = RU_SUSPENDED;
+
  	/* Pull off the RFD and put the actual data (minus eth hdr) */
  	skb_reserve(skb, sizeof(struct rfd));
  	skb_put(skb, actual_size);

@@ -1843,18 +1865,78 @@ static void e100_rx_clean(struct nic *ni
  	unsigned int work_to_do)
  {
  	struct rx *rx;
+	int restart_required = 0;
+	struct rx *rx_to_start = NULL;
+	struct rx *old_before_last_rx, *new_before_last_rx;
+	struct rfd *old_before_last_rfd, *new_before_last_rfd;
+
+	/* are we already rnr? then pay attention!!! this ensures that
+	 * the state machine progression never allows a start with a
+	 * partially cleaned list, avoiding a race between hardware
+	 * and rx_to_clean when in NAPI mode */
+	if(RU_SUSPENDED == nic->ru_running)
+		restart_required = 1;
 
  	/* Indicate newly arrived packets */
  	for(rx = nic->rx_to_clean; rx->skb; rx = nic->rx_to_clean = rx->next) {
-		if(e100_rx_indicate(nic, rx, work_done, work_to_do))
+		int err = e100_rx_indicate(nic, rx, work_done, work_to_do);
+		if(-EAGAIN == err) {
+			/* hit quota so have more work to do, restart once
+			 * cleanup is complete */
+			restart_required = 0;
+			break;
+		} else if(-ENODATA == err)
  			break; /* No more to clean */
  	}
 
+	/* save our starting point as the place we'll restart the receiver */
+	if(restart_required)
+		rx_to_start = nic->rx_to_clean;
+
+	old_before_last_rx = nic->rx_to_use->prev->prev;
+	old_before_last_rfd = (struct rfd *)old_before_last_rx->skb->data;
+
  	/* Alloc new skbs to refill list */
  	for(rx = nic->rx_to_use; !rx->skb; rx = nic->rx_to_use = rx->next) {
  		if(unlikely(e100_rx_alloc_skb(nic, rx)))
  			break; /* Better luck next time (see watchdog) */
  	}
+
+	new_before_last_rx = nic->rx_to_use->prev->prev;
+	if (new_before_last_rx != old_before_last_rx) {
+		/* Set the el-bit on the buffer that is before the last buffer.
+		 * This lets us update the next pointer on the last buffer without
+		 * worrying about hardware touching it.
+		 * We set the size to 0 to prevent hardware from touching this buffer.
+		 * When the hardware hits the before last buffer with el-bit and size
+		 * of 0, it will RNR interrupt, the RUS will go into the No Resources
+		 * state.  It will not complete nor write to this buffer. */
+		new_before_last_rfd = (struct rfd *)new_before_last_rx->skb->data;
+		new_before_last_rfd->size = 0;
+		new_before_last_rfd->command |= cpu_to_le16(cb_el);
+		pci_dma_sync_single_for_device(nic->pdev, new_before_last_rx->dma_addr,
+			sizeof(struct rfd), PCI_DMA_TODEVICE);
+
+		/* Now that we have a new stopping point, we can clear the old
+		 * stopping point.
+		 * Note: There appears to be a race here where the hardware
+		 * can complete this buffer with the el-bit set but with the
+		 * size also set.  The hardware RNR interrupts, the RUS
+		 * goes into the No Resources state. */
+		old_before_last_rfd->command &= ~cpu_to_le16(cb_el);
+		wmb();
+		old_before_last_rfd->size = cpu_to_le16(VLAN_ETH_FRAME_LEN);
+		pci_dma_sync_single_for_device(nic->pdev, old_before_last_rx->dma_addr,
+			sizeof(struct rfd), PCI_DMA_TODEVICE);
+	}
+
+	if(restart_required) {
+		// ack the rnr?
+		writeb(stat_ack_rnr, &nic->csr->scb.stat_ack);
+		e100_start_receiver(nic, rx_to_start);
+		if(work_done)
+			(*work_done)++;
+	}
  }
 
  static void e100_rx_clean_list(struct nic *nic)

@@ -1862,6 +1944,8 @@ static void e100_rx_clean_list(struct ni
  	struct rx *rx;
  	unsigned int i, count = nic->params.rfds.count;
 
+	nic->ru_running = RU_UNINITIALIZED;
+
  	if(nic->rxs) {
  		for(rx = nic->rxs, i = 0; i < count; rx++, i++) {
  			if(rx->skb) {

@@ -1881,8 +1965,10 @@ static int e100_rx_alloc_list(struct nic
  {
  	struct rx *rx;
  	unsigned int i, count = nic->params.rfds.count;
+	struct rfd *before_last;
 
  	nic->rx_to_use = nic->rx_to_clean = NULL;
+	nic->ru_running = RU_UNINITIALIZED;
 
  	if(!(nic->rxs = kcalloc(count, sizeof(struct rx), GFP_ATOMIC)))
  		return -ENOMEM;

@@ -1896,7 +1982,22 @@ static int e100_rx_alloc_list(struct nic
  		}
  	}
 
+	/* Set the el-bit on the buffer that is before the last buffer.
+	 * This lets us update the next pointer on the last buffer without
+	 * worrying about hardware touching it.
+	 * We set the size to 0 to prevent hardware from touching this buffer.
+	 * When the hardware hits the before last buffer with el-bit and size
+	 * of 0, it will RNR interrupt, the RUS will go into the No Resources
+	 * state.  It will not complete nor write to this buffer. */
+	rx = nic->rxs->prev->prev;
+	before_last = (struct rfd *)rx->skb->data;
+	before_last->command |= cpu_to_le16(cb_el);
+	before_last->size = 0;
+	pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+		sizeof(struct rfd), PCI_DMA_TODEVICE);
+
  	nic->rx_to_use = nic->rx_to_clean = nic->rxs;
+	nic->ru_running = RU_SUSPENDED;
 
  	return 0;
  }

@@ -1916,6 +2017,10 @@ static irqreturn_t e100_intr(int irq, vo
  	/* Ack interrupt(s) */
  	iowrite8(stat_ack, &nic->csr->scb.stat_ack);
 
+	/* We hit Receive No Resource (RNR); restart RU after cleaning */
+	if(stat_ack & stat_ack_rnr)
+		nic->ru_running = RU_SUSPENDED;
+
  	if(likely(netif_rx_schedule_prep(netdev))) {
  		e100_disable_irq(nic);
  		__netif_rx_schedule(netdev);

@@ -2007,7 +2112,7 @@ static int e100_up(struct nic *nic)
  	if((err = e100_hw_init(nic)))
  		goto err_clean_cbs;
  	e100_set_multicast_list(nic->netdev);
-	e100_start_receiver(nic);
+	e100_start_receiver(nic, NULL);
  	mod_timer(&nic->watchdog, jiffies);
  	if((err = request_irq(nic->pdev->irq, e100_intr, IRQF_SHARED,
  		nic->netdev->name, nic->netdev)))

@@ -2088,7 +2193,7 @@ static int e100_loopback_test(struct nic
  		mdio_write(nic->netdev, nic->mii.phy_id, MII_BMCR,
  			BMCR_LOOPBACK);
 
-	e100_start_receiver(nic);
+	e100_start_receiver(nic, NULL);
 
  	if(!(skb = netdev_alloc_skb(nic->netdev, ETH_DATA_LEN))) {
  		err = -ENOMEM;
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · David Acker

Quoted 1 line · David Acker

Done. Below is a patch against 2.6.22-rc1. It combines removing the s-bit patch and applying the patch I previously sent.

Oops. I missed one state in that patch. Since the el-bit buffer will normally not complete due to a zero size, we need to check if the buffer with no data has the el-bit set. Without this, you have to wait for the interrupt. Sorry about that...this was in the code I tested on my embedded system but got lost in the regular kernel patch.

OK. Thanks.

If you don't mind I'm going to have some testing on this patch done for a bit now (mostly x86 hardware of course) to see if there's no pitfalls in it. It'll be a few days because of the weekend before I get back on it.

Quoted 8 lines · Auke

Quoted 3 lines · David Acker

Quoted 1 line · David Acker

Done. Below is a patch against 2.6.22-rc1. It combines removing the s-bit patch and applying the patch I previously sent.

Oops. I missed one state in that patch. Since the el-bit buffer will normally not complete due to a zero size, we need to check if the buffer with no data has the el-bit set. Without this, you have to wait for the interrupt. Sorry about that...this was in the code I tested on my embedded system but got lost in the regular kernel patch.

OK. Thanks.

If you don't mind I'm going to have some testing on this patch done for a bit now (mostly x86 hardware of course) to see if there's no pitfalls in it. It'll be a few days because of the weekend before I get back on it.

Cool. I will see if I can get some more tests running over the weekend on our PXA255 platform.

First impression just came in: It seems RX performance is dropped to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new code seems to misbehave and fluctuate, dropping below 10mbit after a few netperf runs and staying there...

ideas?

Quoted 2 lines · Auke

First impression just came in: It seems RX performance is dropped to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new code seems to misbehave and fluctuate, dropping below 10mbit after a few netperf runs and staying there... ideas?

I found the problem. Another casualty of working with two different kernels at once...arg. The blank rfd needs to have its el-bit clear now. Here is the new and improved patch.

On the ARM, their is a race condition between software allocating a new receive buffer and hardware writing into a buffer. The two race on touching the last Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal to 0. When hardware encounters this buffer it attempts to write data to it and then update Status Word bits and Actual Count in the RFD. At the same time software may try to clear the el-bit and set the link address to a new buffer. Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer in the chain. When the hardware encounters this buffer it stops and does not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers, it can update the tail of the list because it knows the hardware will stop at the buffer before it. Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer. If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and complete that one in error. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Signed-off-by David Acker <[email protected]>

Quoted 30 lines · David Acker

Quoted 2 lines · Auke

First impression just came in: It seems RX performance is dropped to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new code seems to misbehave and fluctuate, dropping below 10mbit after a few netperf runs and staying there... ideas?

I found the problem. Another casualty of working with two different kernels at once...arg. The blank rfd needs to have its el-bit clear now. Here is the new and improved patch.

On the ARM, their is a race condition between software allocating a new receive buffer and hardware writing into a buffer. The two race on touching the last Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal to 0. When hardware encounters this buffer it attempts to write data to it and then update Status Word bits and Actual Count in the RFD. At the same time software may try to clear the el-bit and set the link address to a new buffer. Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer in the chain. When the hardware encounters this buffer it stops and does not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers, it can update the tail of the list because it knows the hardware will stop at the buffer before it. Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer. If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and complete that one in error. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Quoted 2 lines

Signed-off-by David Acker <[email protected]>

This patch doesn't apply. It appears white space damaged somewhere:

(1) blank lines in diff are empty not <space>
(2) unchanged lines starting with tab are <space><space><tab>

After fixing the above I still get:

patching file drivers/net/e100.c
Hunk #1 FAILED at 285.
Hunk #4 FAILED at 1749.
Hunk #8 FAILED at 1865.
Hunk #10 succeeded at 1965 with fuzz 1.
Hunk #11 succeeded at 1982 with fuzz 1.
3 out of 14 hunks FAILED -- saving rejects to file drivers/net/e100.c.rej

although I haven't figured out what is wrong.

Proceeding with the review:

Coding style:
(1) if body on seperate line.
(2) space after if before (
(3) The other enums in this driver are not ALL_CAPS
(4) This driver doesn't do CONSTANT != value but value != enum

     (see nic->mac for examples)

Quoted 6 lines · Milton Miller

Quoted 5 lines · David Acker

Quoted 2 lines · Auke

First impression just came in: It seems RX performance is dropped to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new code seems to misbehave and fluctuate, dropping below 10mbit after a few netperf runs and staying there... ideas?

I found the problem. Another casualty of working with two different kernels at once...arg. The blank rfd needs to have its el-bit clear now. Here is the new and improved patch.

Quoted 7 lines

Proceeding with the review:
Coding style:
(1) if body on seperate line.
(2) space after if before (
(3) The other enums in this driver are not ALL_CAPS
(4) This driver doesn't do CONSTANT != value but value != enum

     (see nic->mac for examples)

I sent Milton my copy of this patch which has these style issues corrected and applies cleanly to a recent git tree. If anyone else specifically wants a copy let me know.

Auke

Quoted 8 lines

Quoted 7 lines

Proceeding with the review:
Coding style:
(1) if body on seperate line.
(2) space after if before (
(3) The other enums in this driver are not ALL_CAPS
(4) This driver doesn't do CONSTANT != value but value != enum

     (see nic->mac for examples)

Quoted 5 lines

I sent Milton my copy of this patch which has these style issues corrected and applies cleanly to a recent git tree. If anyone else specifically wants a copy let me know.

Auke

It addressed 1 and 2, and applies, but did not address 3 and 4.

But the bigger point is it didn't address the holes I identified.

I also see that the driver sets the size from 0 back to frame size. While there is a wmb() between the removal of EL and the restore of the frame size, there is only one pci_sync for both of them. Since the size is in a separate word, depending on skb alignment it may be in a different cache line and we may end up with all orderings being visible to the device.

This patch adds a lot of code that checks if the next RFD has EL set and assumes that if it sees the next frame to clean has EL set then the device will have seen it and entered the stopped
state. In theory, the device might be held off on the PCI bus
and not have yet read the descriptor and stopped. In addition,
while it anticipates the RNR interrupt and restarts the receiver from the clean process, it doesn't clear the pending interrupt and the cpu will still take the interrupt, although it will read the status and not actually restart the RU.

I think we need to change the logic to reclaim the size from 0 only if we are restarting, and make rx_indicate look ahead to rx->next if it encounters a !EL size 0 buffer. Without this we are doing a "prohibited" rx_start to a running machine.

The
device can still see this size 0 !EL state. Also we will get stuck when the device finds the window between the two writes.

This window also means that the device may have skipped this previously 0-length descriptor and gone on to the next one, so we will stick waiting for the device to write to this descriptor while it went on to the next one.

We can remove some register pressure by finding old_before_last_rfd when we are ready to use it, just comparing old_before_last_rx to new.

Also, as I pointed out, the rx_to_start change to start_reciever is compicated and unnecessary, as rx_to_clean can always be used and it was the starting point before the changes.

As far as the RU_SUSPENDED, I don't think we need it, instead we should poll the device.

Here is my proposal:
rx_indicate can stop when it hits the packet with EL. If it hits a packet with size 0, look ahead to rx->next to see if it is complete, if so complete this one otherwise leave it as next to clean. After the rx_indicate loop, try to allocate more skbs. If we are successful, then fixup the before-next as we do now. Then check the device status for RNR, and if its stopped then set rx_to_clean rfd size back to ETH_LEN and restart the reciever.

This does have a small hole: if we only add one packet at a time we will end up with all size 0 descriptors in the
lopp. We can detect that and not remove EL from the old
before-next unless we are restarting. That would imply moving the status poll before we allocate the list.

Milton Miller wrote:

Many of the issues you bring have been in the e100 for some time. If you ignore the s-bit patch, I basically did the the following:
moved the el-bit to before the last buffer so that the last buffer was protected during chaining.
set the el-bit buffer (next-to-last) size to 0 so that it is not written to while we are writing to it.

This seemed the only way to protect the buffers we are changing while having code to stay ahead of the hardware.

Quoted 8 lines

But the bigger point is it didn't address the holes I identified.

I also see that the driver sets the size from 0 back to frame size. While there is a wmb() between the removal of EL and the restore of the frame size, there is only one pci_sync for both of them. Since the size is in a separate word, depending on skb alignment it may be in a different cache line and we may end up with all orderings being visible to the device.

Hmmm...interesting. The start of the data of the skb may not be aligned on a cache line. Ok DMA experts...what happens when you sync across cache lines? It should dump both of them, right? I guess the orderings could vary...hence why I saw completions with size set but the el-bit still set. So perhaps the skb alloc needs to be aligned by cache line? This issue existed before my patches as well.

Quoted 9 lines

This patch adds a lot of code that checks if the next RFD has EL set and assumes that if it sees the next frame to clean has EL set then the device will have seen it and entered the stopped
state. In theory, the device might be held off on the PCI bus
and not have yet read the descriptor and stopped. In addition,
while it anticipates the RNR interrupt and restarts the receiver from the clean process, it doesn't clear the pending interrupt and the cpu will still take the interrupt, although it will read the status and not actually restart the RU.

So the device is just before the el-bit buffer and stops. We get a poll call with interrupts disabled and see the el-bit buffer and decide we need to restart. First we alloc new buffers and move the el-bit/size 0 down. The restart occurs on the next poll (interrupts are not turned on if we did any work). In theory, the hardware could have read the buffer that no longer has a size of 0 and tried to use it when we hit it with the start. I am not sure how the hardware handles this. Perhaps it is ignored and thus harmless. We could poll the hardware to check before actually sending the start...it adds an extra pci operation but would avoid a command that is illegal in the manual.

If we ignore a buffer with the el-bit set but without the complete bit, we must wait for the hardware to RNR interrupt. I have found that when a buffer has both the el-bit set and the size set to 0, it will not set the complete bit on that buffer. This was part of the reason why my first patch had hardware just using the list of buffers given all the way up until the end.

Quoted 4 lines

I think we need to change the logic to reclaim the size from 0 only if we are restarting, and make rx_indicate look ahead to rx->next if it encounters a !EL size 0 buffer. Without this we are doing a "prohibited" rx_start to a running machine.

The hardware is only restarted when we enter the RU_SUSPENDED state. This only happens when we:
1) get an RNR interrupt
2) get an EL-bit buffer without a completion
3) get an el-bit buffer with a completion (hardware saw size set but not el-bit clear)

State 2) seems to be the problem. Get rid of that and we wait for an interrupt to tell us it saw the buffer in most cases. Of course, then we always wait.

If we do not reclaim the size from 0, but clear the el-bit, that buffer will always return an error.

Quoted 3 lines

The device can still see this size 0 !EL state. Also we will get stuck when the device finds the window between the two writes.

Quoted 4 lines

This window also means that the device may have skipped this previously 0-length descriptor and gone on to the next one, so we will stick waiting for the device to write to this descriptor while it went on to the next one.

If the hardware sees both size 0 and EL, it RNR interrupts, does not write to the buffer at all and stops.

If the hardware sees size 0 and !EL, (which I never saw in testing but could happen) it would return the buffer with an error and continue. This is ok since we already have written a new size 0 and EL. Our polling code would just find a packet completed in error.

If the hardware sees size set and EL, it RNR interrupts, and does write data and status to the buffer, and then stops. I have see this one occur and it isn't a problem.

If hardware sees size set and !EL, it writes data and continues.

Quoted 2 lines

As far as the RU_SUSPENDED, I don't think we need it, instead we should poll the device.

RU_SUSPENDED is pretty much used as it was before all the patches. The only waste I found is that if we find the EL-bit while interrupts are disabled (say during a NAPI poll), we set RU_SUSPENDED and then have to wait for the next poll to figure out that we need to restart the receiver.

Quoted 6 lines

Here is my proposal:
rx_indicate can stop when it hits the packet with EL. If it hits a packet with size 0, look ahead to rx->next to see if it is complete, if so complete this one otherwise leave it as next to clean. After the rx_indicate loop, try to allocate more skbs. If we are successful, then fixup the before-next

Quoted 8 lines

as we do now. Then check the device status for RNR, and if its stopped then set rx_to_clean rfd size back to ETH_LEN and restart the reciever.
This does have a small hole: if we only add one packet at a time we will end up with all size 0 descriptors in the
lopp. We can detect that and not remove EL from the old
before-next unless we are restarting. That would imply moving the status poll before we allocate the list.

Can you send a patch or pseudo-code? It would help to see some main parts:
1. blank_rfd setup
2. single buffer alloc
3. el-bit handling (when do we set it and when do we clear it)
4. size handling (when do we set it and when do we clear it)

What if we just fixed the alignment of rfd to make sure it is cache aligned? Then we know our syncs will be a single cache flush.

I agree with this part of the approach. I just think we need a bit more work on the "what to do when we are ready for hardware to not stop" part.

The sync is required to push both cache lines, but there is no ordering guarantee. This probably is why you saw size and el set. Aligning the RFD to a cache line conflicts with aligning the payload (IP header and data) to a word boundary, and depending on cache line size it may be impossible to do both.

And it won't fix the hole for coherent dma machines.

It wasn't an issue before because we never set two fields.

My current reading of the manual is that the C bit will not be set on an RFD that is size 0. It goes on to processes EL and S, and decides to stop and interrupt RNR or suspend, or just go to the next packet.

Quoted 4 lines · Milton Miller

My current reading of the manual is that the C bit will not be set on an RFD that is size 0. It goes on to processes EL and S, and decides to stop and interrupt RNR or suspend, or just go to the next packet.

I double checked this with a quick experiment and it appears you are correct.

What about if we always did the following:
set the size:
sync();
clear el-bit
sync()

Then if the hardware sees just the size set, the packet completes but with the el-bit and we know we need to restart since it completed. If it sees the size == 0, and the el bit set, it stops and RNR interrupts.

When we find a buffer that is not completed but has the el-bit set, we read the status byte of the status control block to see if the RU is in the no resources state. If it isn't, it means that we found that buffer before the hardware did and thus need to wait for it. We will either find it on the next poll or enable interrupts and get told about it by hardware.

What do you think?

Quoted 9 lines · David Acker

Quoted 4 lines · Milton Miller

My current reading of the manual is that the C bit will not be set on an RFD that is size 0. It goes on to processes EL and S, and decides to stop and interrupt RNR or suspend, or just go to the next packet.

I double checked this with a quick experiment and it appears you are correct.
When we find a buffer that is not completed but has the el-bit set, we read the status byte of the status control block to see if the RU is in the no resources state. If it isn't, it means that we found that buffer before the hardware did and thus need to wait for it. We will either find it on the next poll or enable interrupts and get told about it by hardware.

What do you think?

I think the second part is good ...

Ok here's my just-before-dinner brainstorming, as relayed after dinner:

We add two flags to struct rx: one says this packet is EL, and one says
it is or was size 0. We create a function, find_mark_el(struct nic,
is_running) that is called after initial alloc and/or after refilling
skb list. In find_mark_el, we start with rx_to_use (lets rename this
rx_to_fill), and go back two entries, call this rx_to_mark. If
is_running and rx_to_mark->prev->el then just return, leave EL alone. Otherwise, set EL and size to 0, set the two flags in the rx struct, sync the RFD, then search for the current EL (we could save the pointer, but its always the odl rx_to_use (fill) or its ->prev). Clear RFD->EL, sync, clear rx->el. Set size non-zero, sync, Leave the was_0 flag set if is_running (clear it only if we know reciever is stopped).

At this point, if the receiver was stopped we can restart it,. We should do so in the caller. (We always restart at rx_to_clean). Restart should ack the RNR status before issuing the ru_start command to avoid a spurious RNR interrupt.

In the receive loop, if RFD->C is not set, rx->was_0 is set and el is not set, then we need to check rx->next->RFD->C bit (after
pci_sync_for_cpu). If the next packet C bit is set then we consider
this skb as used, and just complete it with no data to the stack.

Because find_mark_el will only advance EL if the receiver was stopped or we had more than 1 packet added, we can guarantee that if packet N has was_0 set, then packet N+1 will not have it set, so we have bounded lookahead.

This adds two flags to struct rx, but that is allocated as a single array and the array size is filled out as forward and back pointers. Rx clean only has to look at was_0 on the last packet when it decides C is not set, and only if both are set does it peek at the next packet. In other words, we shouldn't worry about the added flags making it a non-power-of-2 size.

By setting size != 0 after we have modified all other fields, the device will only write to this packet if we are done writing. If we loose the race and only partially complete, it will just go on to the next packet and we find it. If we totally loose, then the receiver will go RNR and we can reclaim all the buffer space we have allocated.

Comments? Questions?

We need to enforce a minimum rx ring size (3? 4?).

We rely on other mechanisms to guarantee the RFD in this skb will not cache line conflict with the data in antoher scb (slab allocs of the skb should give us this).

Quoted 36 lines

Ok here's my just-before-dinner brainstorming, as relayed after dinner:

We add two flags to struct rx: one says this packet is EL, and one says
it is or was size 0. We create a function, find_mark_el(struct nic,
is_running) that is called after initial alloc and/or after refilling
skb list. In find_mark_el, we start with rx_to_use (lets rename this
rx_to_fill), and go back two entries, call this rx_to_mark. If
is_running and rx_to_mark->prev->el then just return, leave EL alone. Otherwise, set EL and size to 0, set the two flags in the rx struct, sync the RFD, then search for the current EL (we could save the pointer, but its always the odl rx_to_use (fill) or its ->prev). Clear RFD->EL, sync, clear rx->el. Set size non-zero, sync, Leave the was_0 flag set if is_running (clear it only if we know reciever is stopped).

At this point, if the receiver was stopped we can restart it,. We should do so in the caller. (We always restart at rx_to_clean). Restart should ack the RNR status before issuing the ru_start command to avoid a spurious RNR interrupt.

In the receive loop, if RFD->C is not set, rx->was_0 is set and el is not set, then we need to check rx->next->RFD->C bit (after
pci_sync_for_cpu). If the next packet C bit is set then we consider
this skb as used, and just complete it with no data to the stack.

Because find_mark_el will only advance EL if the receiver was stopped or we had more than 1 packet added, we can guarantee that if packet N has was_0 set, then packet N+1 will not have it set, so we have bounded lookahead.

This adds two flags to struct rx, but that is allocated as a single array and the array size is filled out as forward and back pointers. Rx clean only has to look at was_0 on the last packet when it decides C is not set, and only if both are set does it peek at the next packet. In other words, we shouldn't worry about the added flags making it a non-power-of-2 size.

Quoted 8 lines

By setting size != 0 after we have modified all other fields, the device will only write to this packet if we are done writing. If we loose the race and only partially complete, it will just go on to the next packet and we find it. If we totally loose, then the receiver will go RNR and we can reclaim all the buffer space we have allocated.

Comments? Questions?

Quoted 1 line

We need to enforce a minimum rx ring size (3? 4?).

Quoted 3 lines

We rely on other mechanisms to guarantee the RFD in this skb will not cache line conflict with the data in antoher scb (slab allocs of the skb should give us this).

Copying EL to a flag in rx is only to avoid additional reading of the rfd while it might be under dma. We do need the was_0 flag.

Do we need to continue with the stop-before-last plan? As long as we set size to 0 with EL, we shoud be able to change the link, sync, set size 0, sync, and then set size.

We still need the "advance at least 2 if not stopped" check when deciding to move the EL. This would break if the hardware in the dma path can read the multiple cache lines of the RFD out of order, so it may not be safe (if some pci host decided to prefetch data, and got the second line ahead of the first and didn't discard prefetch until pci bus disconnect). Actually, I am afraid I know hardware that might do that.

[defer]
Currently we handle failed allocs by doing a sw interrupt in the watchdog. Since we fail ifup if we can't alloc rxs, we can always start the reciever, even if we didn't alloc a new packet -- it will just read the RFD and go RNR
again. We could make this sw interrupt conditional on
rx_to_clean->el being set. However, looking further, it
appears this 2s watchdog induced watchdog also covers makeing sure that we reattempt netif_schedule_prep_rx when
it fails in e100_intr. Any change in this area should be
in a seperate patch, and probably delayed at least one
release. I also note that netpoll_controller calls
e100_intr, which will call into the netif_rx_schedule only when a device interrupt is active.

Quoted 1 line · Milton Miller

Comments? Questions?

This sounds pretty reasonable. I will take a stab at coding this up today; I always think better looking at code.

Quoted 3 lines

Do we need to continue with the stop-before-last plan? As long as we set size to 0 with EL, we shoud be able to change the link, sync, set size 0, sync, and then set size.

Perhaps not. I can take a try at coding it without it. It would certainly make the driver easier to follow.

Quoted 8 lines

We still need the "advance at least 2 if not stopped" check when deciding to move the EL. This would break if the hardware in the dma path can read the multiple cache lines of the RFD out of order, so it may not be safe (if some pci host decided to prefetch data, and got the second line ahead of the first and didn't discard prefetch until pci bus disconnect). Actually, I am afraid I know hardware that might do that.

Hmm, me too.

Quoted 6 lines

We add two flags to struct rx: one says this packet is EL, and one says
it is or was size 0. We create a function, find_mark_el(struct nic,
is_running) that is called after initial alloc and/or after refilling
skb list. In find_mark_el, we start with rx_to_use (lets rename this
rx_to_fill), and go back two entries, call this rx_to_mark. If
is_running and rx_to_mark->prev->el then just return, leave EL alone.

Quoted 11 lines

Otherwise, set EL and size to 0, set the two flags in the rx struct, sync the RFD, then search for the current EL (we could save the pointer, but its always the odl rx_to_use (fill) or its ->prev). Clear RFD->EL, sync, clear rx->el. Set size non-zero, sync, Leave the was_0 flag set if is_running (clear it only if we know reciever is stopped).

At this point, if the receiver was stopped we can restart it,. We should do so in the caller. (We always restart at rx_to_clean). Restart should ack the RNR status before issuing the ru_start command to avoid a spurious RNR interrupt.

Quoted 4 lines

In the receive loop, if RFD->C is not set, rx->was_0 is set and el is not set, then we need to check rx->next->RFD->C bit (after
pci_sync_for_cpu). If the next packet C bit is set then we consider
this skb as used, and just complete it with no data to the stack.

Quoted 4 lines

Because find_mark_el will only advance EL if the receiver was stopped or we had more than 1 packet added, we can guarantee that if packet N has was_0 set, then packet N+1 will not have it set, so we have bounded lookahead.

Quoted 14 lines

This adds two flags to struct rx, but that is allocated as a single array and the array size is filled out as forward and back pointers. Rx clean only has to look at was_0 on the last packet when it decides C is not set, and only if both are set does it peek at the next packet. In other words, we shouldn't worry about the added flags making it a non-power-of-2 size.

By setting size != 0 after we have modified all other fields, the device will only write to this packet if we are done writing. If we loose the race and only partially complete, it will just go on to the next packet and we find it. If we totally loose, then the receiver will go RNR and we can reclaim all the buffer space we have allocated.

Quoted 2 lines

Comments? Questions?

Ok, I finally got some time to code this out and study it and Ihave some questions.

Quoted 6 lines · Milton Miller

Quoted 6 lines

We add two flags to struct rx: one says this packet is EL, and one says
it is or was size 0. We create a function, find_mark_el(struct nic,
is_running) that is called after initial alloc and/or after refilling
skb list. In find_mark_el, we start with rx_to_use (lets rename this
rx_to_fill), and go back two entries, call this rx_to_mark. If
is_running and rx_to_mark->prev->el then just return, leave EL alone.

So if we start clean and then one packet completes, we can not clear the old marked entry? We must then add a per nic pointer to the current marked entry so that when the next packet completes, we can avoid scanning for it. Why do we need to check this again?

Quoted 12 lines

Quoted 12 lines

Otherwise, set EL and size to 0, set the two flags in the rx struct, sync the RFD, then search for the current EL (we could save the pointer, but its always the odl rx_to_use (fill) or its ->prev). Clear RFD->EL, sync, clear rx->el. Set size non-zero, sync, Leave the was_0 flag set if is_running (clear it only if we know reciever is stopped).

At this point, if the receiver was stopped we can restart it,. We should do so in the caller. (We always restart at rx_to_clean). Restart should ack the RNR status before issuing the ru_start command to avoid a spurious RNR interrupt.

Quoted 4 lines

Quoted 4 lines

In the receive loop, if RFD->C is not set, rx->was_0 is set and el is not set, then we need to check rx->next->RFD->C bit (after
pci_sync_for_cpu). If the next packet C bit is set then we consider
this skb as used, and just complete it with no data to the stack.

If the C-bit is not set, we can read the status to see if the RU is running (we cleared the EL bit before it read it but it has not found another packet yet) or not (it read the el-bit before we cleared it). This read lets us avoid going through an enable interrupts, wait for the RNR interrupt cycle.

Quoted 4 lines

Quoted 4 lines

Because find_mark_el will only advance EL if the receiver was stopped or we had more than 1 packet added, we can guarantee that if packet N has was_0 set, then packet N+1 will not have it set, so we have bounded lookahead.

Is this meant to be 1 packet added at any given call or 1 packet added since the last time we cleared?

Quoted 14 lines

Quoted 14 lines

This adds two flags to struct rx, but that is allocated as a single array and the array size is filled out as forward and back pointers. Rx clean only has to look at was_0 on the last packet when it decides C is not set, and only if both are set does it peek at the next packet. In other words, we shouldn't worry about the added flags making it a non-power-of-2 size.

By setting size != 0 after we have modified all other fields, the device will only write to this packet if we are done writing. If we loose the race and only partially complete, it will just go on to the next packet and we find it. If we totally loose, then the receiver will go RNR and we can reclaim all the buffer space we have allocated.

Quoted 2 lines

Quoted 2 lines

Comments? Questions?

Say we have an 8 buffer receive pool (0-7) at start. rx[6] is marked.

hardware consumes rx[0]
software sees one rx complete without an s-bit set.

software notes that rx[6] is marked
software allocates new buffer for rx[0]
software runs find_mark_el with rx_to_use == rx[1], rx_to_mark == rx[7], is_running == true, marked_rx == rx[6] find_mark_el finds rx_to_mark->prev->el set and is_running true, and returns.

hardware consumes rx[1]
software sees one rx complete without an s-bit set.

software notes that rx[6] is still marked
software allocates new buffer for rx[1]
software runs find_mark_el with rx_to_use == rx[2], rx_to_mark == rx[0], is_running == true, marked_rx == rx[6] find_mark_el does not find rx_to_mark->prev->el set so continues
find_mark_el sets rx[0]->rfd->el rx[0]->rfd->size = 0, rx[0]->el, rx[0]->size0;
find_mark_el clears rx[6]->rfd->el, syncs, sets rx[6]->rfd->size, syncs, clears rx[6]->rfd->el but leaves rx[6]->rfd->size0 set.

Is this the correct flow?

hardware consumes rx[1]
software sees one rx complete without an s-bit set.

software allocates new buffer for rx[1], and moves rx_to_use to rx[2]

Your logic works, this adds some conditional branching but saves a pointer, not sure which is better. Mine can be used for initializing without special casing since it will just calculate rx_to_unmark as rx[n-2] and rx_to_mark as rx[n-2] != rx[n-2]->prev; unmarking a never marked still works, where as for yours the "was marked" must be explicitly set to something (hmm, rxs = rx[0] might work for that initial value until we mark -2??)

again, the compare of rx->el instead of rx->rfd->el is to save cache traffic to the rfd under io. The rx->was_0 is required, so the el flag is free.

Quoted 8 lines · Milton Miller

Your logic works, this adds some conditional branching but saves a pointer, not sure which is better. Mine can be used for initializing without special casing since it will just calculate rx_to_unmark as rx[n-2] and rx_to_mark as rx[n-2] != rx[n-2]->prev; unmarking a never marked still works, where as for yours the "was marked" must be explicitly set to something (hmm, rxs = rx[0] might work for that initial value until we mark -2??)

Quoted 4 lines

again, the compare of rx->el instead of rx->rfd->el is to save cache traffic to the rfd under io. The rx->was_0 is required, so the el flag is free.

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3.
Let me know what you think. Testing shows that we can hit any of the following scenarios:

Find a buffer not complete with rx->el and rx->s0 set. I read back the status and see if
the receiver is still running.
Find a buffer not complete with rx->el not set and rx->s0 set. I check the next buffer
and if it is complete, I free the skb and return 0. If the next buffer is not
complete, I read the receiver's status to see if he is still running. Find a buffer that is complete with rx->el not set and rx->s0 set. It appears that hardware
can read the rfd's el-bit, then software can clear the rfd el-bit and set the rfd size, and
then hardware can come in and read the size. I am reading the status back, although
I don't think that I have to in this instance.

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

On the ARM, their is a race condition between software allocating a new receive
buffer and hardware writing into a buffer. The two race on touching the last
Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal
to 0. When hardware encounters this buffer it attempts to write data to it and then update Status Word bits and Actual Count in the RFD. At the same time
software may try to clear the el-bit and set the link address to a new buffer.
Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers
getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer
in the chain. When the hardware encounters this buffer it stops and does not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers, it can update the tail of the list when either it knows the hardware has stopped or the previous to the new one to mark marked. Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer. We keep flags in our software descriptor to note if the el bit is set and if the size was 0. When we clear the RFD's el bit and set its size, we also clear the el flag but we leave the size was 0 bit set. This was we can find this buffer again later.

If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and skip this one. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Signed-off-by David Acker <[email protected]>

Quoted 1 line · David Acker

Milton Miller wrote:

Quoted 9 lines

the el flag but we leave the size was 0 bit set. This was we can find this buffer again later.

If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and skip this one. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Quoted 1 line

Signed-off-by David Acker <[email protected]>

That seems to vaguely match my memory of what eepro100 was doing (or trying to do).

I _really_ appreciate you working on this problem. Getting e100 driver stable for the long term, and ditching eepro100, is a big hurdle to cross. Getting this right is really one of the last steps.

The patch looks OK at quick glance.

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3.
Let me know what you think.

Testing shows that we can hit any of the following scenarios:

Find a buffer not complete with rx->el and rx->s0 set.
I read back the status and see if the receiver is still running.

Find a buffer not complete with rx->el not set and rx->s0 set.
I check the next buffer and if it is complete, I free the skb and
return 0.

If the next buffer is not complete, I read the receiver's status to see if he is still running.

Find a buffer that is complete with rx->el not set and rx->s0 set.
It appears that hardware can read the rfd's el-bit, then software can
clear the rfd el-bit and set the rfd size, and then hardware can come in and read the size.

I am reading the status back, although I don't think that I have to in this instance.

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

On the ARM, their is a race condition between software allocating a new receive
buffer and hardware writing into a buffer. The two race on touching the last
Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal
to 0. When hardware encounters this buffer it attempts to write data to it
and then update Status Word bits and Actual Count in the RFD. At the same time
software may try to clear the el-bit and set the link address to a new buffer.
Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers
getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer
in the chain. When the hardware encounters this buffer it stops and does
not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers,
it can update the tail of the list when either it knows the hardware has stopped or the previous to the new one to mark marked. Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer. We keep flags in our software descriptor to note if the el bit is set and if the size was 0. When we clear the RFD's el bit and set its size, we also clear the el flag but we leave the size was 0 bit set. This was we can find this buffer again later.

If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and skip this one. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

Signed-off-by David Acker <[email protected]>

---

--- linux-2.6.22-rc3/drivers/net/e100.c.orig 2007-06-01
16:13:16.000000000 -0400
+++ linux-2.6.22-rc3/drivers/net/e100.c 2007-06-01 16:20:36.000000000
-0400

drivers/net/e100.c renamed from drivers/net/e100.c.orig +1 -0

@@ -281,10 +281,17 @@ struct csr {
 };
 
 enum scb_status {
+	rus_no_res       = 0x08,

drivers/net/e100.c renamed from drivers/net/e100.c.orig +8 -1

 enum scb_stat_ack {
 	stat_ack_not_ours    = 0x00,
 	stat_ack_sw_gen      = 0x04,

@@ -395,10 +402,16 @@ struct rfd {
 	u16 size;
 };
 
+enum rx_flags {
+	rx_el = 0x01,
+	rx_s0 = 0x02,
+};
+
 struct rx {
 	struct rx *next, *prev;
 	struct sk_buff *skb;
 	dma_addr_t dma_addr;
+	u8 flags;
 };
 
 #if defined(__BIG_ENDIAN_BITFIELD)

@@ -526,6 +539,7 @@ struct nic {
 	struct rx *rx_to_use;
 	struct rx *rx_to_clean;
 	struct rfd blank_rfd;
+	enum ru_state ru_running;
 
 	spinlock_t cb_lock			____cacheline_aligned;
 	spinlock_t cmd_lock;

@@ -947,7 +961,7 @@ static void e100_get_defaults(struct nic
 		((nic->mac >= mac_82558_D101_A4) ? cb_cid : cb_i));
 
 	/* Template for a freshly allocated RFD */
-	nic->blank_rfd.command = cpu_to_le16(cb_el & cb_s);
+	nic->blank_rfd.command = 0;
 	nic->blank_rfd.rbd = 0xFFFFFFFF;
 	nic->blank_rfd.size = cpu_to_le16(VLAN_ETH_FRAME_LEN);
 

@@ -1742,11 +1756,55 @@ static int e100_alloc_cbs(struct nic *ni
 	return 0;
 }

drivers/net/e100.c renamed from drivers/net/e100.c.orig +6 -0

+
+	/* (Re)start RU if suspended or idle and RFA is non-NULL */
+	if(rx->skb) {
+		e100_exec_cmd(nic, ruc_start, rx->dma_addr);
+		nic->ru_running = ru_running;
+	}

}

#define RFD_BUF_LEN (sizeof(struct rfd) + VLAN_ETH_FRAME_LEN)

drivers/net/e100.c renamed from drivers/net/e100.c.orig +40 -4

@@ -1774,8 +1832,6 @@ static int e100_rx_alloc_skb(struct nic
 		struct rfd *prev_rfd = (struct rfd *)rx->prev->skb->data;
 		put_unaligned(cpu_to_le32(rx->dma_addr),
 			(u32 *)&prev_rfd->link);
-		wmb();
-		prev_rfd->command &= ~cpu_to_le16(cb_el & cb_s);
 		pci_dma_sync_single_for_device(nic->pdev, rx->prev->dma_addr,
 			sizeof(struct rfd), PCI_DMA_TODEVICE);
 	}

@@ -1789,6 +1845,7 @@ static int e100_rx_indicate(struct nic *
 	struct sk_buff *skb = rx->skb;
 	struct rfd *rfd = (struct rfd *)skb->data;
 	u16 rfd_status, actual_size;
+	u8 status;
 
 	if(unlikely(work_done && *work_done >= work_to_do))
 		return -EAGAIN;

@@ -1800,9 +1857,46 @@ static int e100_rx_indicate(struct nic *
 
 	DPRINTK(RX_STATUS, DEBUG, "status=0x%04X\n", rfd_status);
 
-	/* If data isn't ready, nothing to indicate */
-	if(unlikely(!(rfd_status & cb_complete)))
+	/* If data isn't ready, nothing to indicate
+	 * If both the el and s0 rx flags are set, we have hit the marked
+	 * buffer but we don't know if hardware has seen it so we check
+	 * the status.
+	 * If only the s0 flag is set, we check the next buffer.
+	 * If it is complete, we know that hardware saw the rfd el bit
+	 * get cleared but did not see the rfd size get set so it
+	 * skipped this buffer.  We just return 0 and look at the
+	 * next buffer.
+	 * If only the s0 flag is set but the next buffer is
+	 * not complete, we cleared the el flag as hardware
+	 * hit this buffer. */
+	if(unlikely(!(rfd_status & cb_complete))) {
+		u8 maskedFlags = rx->flags & (rx_el | rx_s0);
+		if(maskedFlags == (rx_el | rx_s0)) {
+			status = ioread8(&nic->csr->scb.status);
+			if(status & rus_no_res) {
+				nic->ru_running = ru_suspended;
+			}
+		} else if(maskedFlags == rx_s0) {
+			struct rx *next_rx = rx->next;
+			struct rfd *next_rfd = (struct rfd *)next_rx->skb->data;
+			pci_dma_sync_single_for_cpu(nic->pdev, next_rx->dma_addr,
+				sizeof(struct rfd), PCI_DMA_FROMDEVICE);
+			if(next_rfd->status & cpu_to_le16(cb_complete)) {
+				pci_unmap_single(nic->pdev, rx->dma_addr,
+					RFD_BUF_LEN, PCI_DMA_FROMDEVICE);
+				dev_kfree_skb_any(skb);
+				rx->skb = NULL;
+				rx->flags &= ~rx_s0;
+				return 0;
+			} else {
+				status = ioread8(&nic->csr->scb.status);
+				if(status & rus_no_res) {
+					nic->ru_running = ru_suspended;
+				}
+			}
+		}
 		return -ENODATA;
+	}
 
 	/* Get actual data size */
 	actual_size = le16_to_cpu(rfd->actual_size) & 0x3FFF;

drivers/net/e100.c renamed from drivers/net/e100.c.orig +9 -1

+		status = ioread8(&nic->csr->scb.status);
+		if(status & rus_no_res) {
+			nic->ru_running = ru_suspended;
+		}
+	}
+
 	/* Pull off the RFD and put the actual data (minus eth hdr) */
 	skb_reserve(skb, sizeof(struct rfd));
 	skb_put(skb, actual_size);

@@ -1842,19 +1945,46 @@ static int e100_rx_indicate(struct nic *
 static void e100_rx_clean(struct nic *nic, unsigned int *work_done,
 	unsigned int work_to_do)
 {
-	struct rx *rx;
+	struct rx *rx, *marked_rx;
+	int restart_required = 0;
+	int err = 0;
 

(quoted diff) +1 -0

+	}
 }

static void e100_rx_clean_list(struct nic *nic)

drivers/net/e100.c renamed from drivers/net/e100.c.orig +2 -0

@@ -1862,6 +1992,8 @@ static void e100_rx_clean_list(struct ni
 	struct rx *rx;
 	unsigned int i, count = nic->params.rfds.count;
 
+	nic->ru_running = ru_uninitialized;
+
 	if(nic->rxs) {
 		for(rx = nic->rxs, i = 0; i < count; rx++, i++) {
 			if(rx->skb) {

drivers/net/e100.c renamed from drivers/net/e100.c.orig +7 -0

 	if(!(nic->rxs = kcalloc(count, sizeof(struct rx), GFP_ATOMIC)))
 		return -ENOMEM;

@@ -1897,6 +2030,9 @@ static int e100_rx_alloc_list(struct nic
 	}
 
 	nic->rx_to_use = nic->rx_to_clean = nic->rxs;
+	nic->ru_running = ru_suspended;
+
+	e100_find_mark_el(nic, NULL, 0);
 
 	return 0;
 }

@@ -1916,6 +2052,10 @@ static irqreturn_t e100_intr(int irq, vo
 	/* Ack interrupt(s) */
 	iowrite8(stat_ack, &nic->csr->scb.stat_ack);
 
+	/* We hit Receive No Resource (RNR); restart RU after cleaning */
+	if(stat_ack & stat_ack_rnr)
+		nic->ru_running = ru_suspended;
+
 	if(likely(netif_rx_schedule_prep(netdev))) {
 		e100_disable_irq(nic);
 		__netif_rx_schedule(netdev);

Quoted 2 lines · David Acker

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3. Let me know what you think.

I think you got most of the ideas. As Auke noted, your coding style is showing again. And your mailer again munged whitespace (fixed by s/^<space><space>/<space>/ s/^$/<space>/).

Quoted 3 lines

Find a buffer that is complete with rx->el not set and rx->s0 set.
It appears that hardware can read the rfd's el-bit, then software
can clear the rfd el-bit and set the rfd size, and then hardware can come in and read the size.

Yes, since the size is after the EL flag in the descriptor, this can happen since the pci read is not atomic.

Quoted 1 line

I am reading the status back, although I don't think that I have to in this instance.

Actually, you are reading it when the rfd still has EL set. Since the cpu will never encounter that case, the if condition is never satisfied.

How about creating a state unknown, for when we think we should check the device if its running. If we are in this state and then encounter a received packet without s0 set, we can set it back
to running. We set it when we rx a packet with s0 set.

We then move both io_status reads to the caller.

Quoted 1 line

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

I'm assuming this is why the cleanup of the receiver start to always start on rx_to_clean got dropped again. :-)

Also, I would like a few sentences in the Driver Operation section IV Receive big comment. Something like

In order to keep updates to the RFD link field from colliding with hardware writes to mark packets complete, we use the feature that hardware will not write to a size 0 descriptor and mark the previous packet as end-of-list (EL). After updating the link, we remove EL and only then restore the size such that hardware may use the previous-to-end RFD.

at the end of the first paragraph, and insert software before "no locking is required" in the second.

Quoted 7 lines · Milton Miller

Quoted 3 lines · David Acker

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3.
Let me know what you think.

I think you got most of the ideas. As Auke noted, your coding style
is showing again. And your mailer again munged whitespace (fixed by
s/^<space><space>/<space>/ s/^$/<space>/).

Sorry about the coding style. I instinctively followed what was there instead of kernel coding convention. I will look into how whitespace is getting screwed up.

Quoted 6 lines

Quoted 4 lines

Find a buffer that is complete with rx->el not set and rx->s0 set.
It appears that hardware can read the rfd's el-bit, then
software can clear the rfd el-bit and set the rfd size, and then hardware can come in and read the size.

Yes, since the size is after the EL flag in the descriptor, this can happen since the pci read is not atomic.

Quoted 5 lines

Quoted 2 lines

I am reading the status back, although I don't think that I have to in this instance.

Actually, you are reading it when the rfd still has EL set. Since the cpu will never encounter that case, the if condition is never satisfied.

In my tests, every time I found a completed rfd with the el-bit set, the receiver was in the out of resources state.

Quoted 6 lines

How about creating a state unknown, for when we think we should check the device if its running.
If we are in this state and then encounter a received packet without s0 set, we can set it back
to running. We set it when we rx a packet with s0 set.
We then move both io_status reads to the caller.

I can look into that as I clean this up.

Quoted 2 lines

Quoted 2 lines

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

The testing I did so far did well. I will try to get some more going tonight, hopefully on a cleaned up patch.

Quoted 4 lines

I'm assuming this is why the cleanup of the receiver start to always start on rx_to_clean got dropped again. :-)

Yep. I will get that in the next patch.

Quoted 10 lines

Also, I would like a few sentences in the Driver Operation section IV Receive big comment. Something like
In order to keep updates to the RFD link field from colliding with hardware writes to mark packets complete, we use the feature that hardware will not write to a size 0 descriptor and mark the previous
packet as end-of-list (EL). After updating the link, we remove EL
and only then restore the size such that hardware may use the previous-to-end RFD.
at the end of the first paragraph, and insert software before "no locking is required" in the second.

Sounds good to me.

I will see if I can get into a cleaned up patch today and get it out by tomorrow. Thanks for dealing with me...I have been around kernel code for awhile but posting official patches to linux is new to me. -Ack

On Jun 5, 2007, at 8:34 AM, David Acker wrote:

Quoted 11 lines

Quoted 7 lines · Milton Miller

Quoted 3 lines · David Acker

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3.
Let me know what you think.

I think you got most of the ideas. As Auke noted, your coding style
is showing again. And your mailer again munged whitespace (fixed by
s/^<space><space>/<space>/ s/^$/<space>/).

Sorry about the coding style. I instinctively followed what was there instead of kernel coding convention. I will look into how whitespace is getting screwed up.

I have to watch my coding style too (I like to indent the closing brace).

At least the white space damage seems to be reversable. More than I can say for this mailer.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Find a buffer that is complete with rx->el not set and rx->s0 set.
It appears that hardware can read the rfd's el-bit, then
software can clear the rfd el-bit and set the rfd size, and then hardware can come in and read the size.

Yes, since the size is after the EL flag in the descriptor, this can happen since the pci read is not atomic.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I am reading the status back, although I don't think that I have to in this instance.

Actually, you are reading it when the rfd still has EL set. Since the cpu will never encounter that case, the if condition is never satisfied.

In my tests, every time I found a completed rfd with the el-bit set, the receiver was in the out of resources state.

Yes, if the EL was set, it would be a real hard race to find the
completed packet with EL but not RNR. I was trying to refer to where
you find a completed packet and then check for EL in the RFD. That is what I was claiming can not be observed by the cpu (unless the card writes the EL bit back, and not just the status u16).

If the unless ... above is true, then please put a comment that the device can write RFD->EL back to 1 if we raced.

Quoted 6 lines

Quoted 6 lines

How about creating a state unknown, for when we think we should check the device if its running.
If we are in this state and then encounter a received packet without s0 set, we can set it back
to running. We set it when we rx a packet with s0 set.
We then move both io_status reads to the caller.

Quoted 7 lines

I can look into that as I clean this up.

Quoted 2 lines

Quoted 2 lines

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

The testing I did so far did well. I will try to get some more going tonight, hopefully on a cleaned up patch.

Good to hear our expectiations match reality.

Quoted 4 lines

Quoted 3 lines

I'm assuming this is why the cleanup of the receiver start to always start on rx_to_clean got dropped again. :-)

Yep. I will get that in the next patch.

Ok.

Quoted 10 lines

Quoted 10 lines

Also, I would like a few sentences in the Driver Operation section IV Receive big comment. Something like
In order to keep updates to the RFD link field from colliding with hardware writes to mark packets complete, we use the feature that hardware will not write to a size 0 descriptor and mark the previous
packet as end-of-list (EL). After updating the link, we remove EL
and only then restore the size such that hardware may use the previous-to-end RFD.
at the end of the first paragraph, and insert software before "no locking is required" in the second.

Quoted 6 lines

Sounds good to me.

I will see if I can get into a cleaned up patch today and get it out by tomorrow. Thanks for dealing with me...I have been around kernel code for awhile but posting official patches to linux is new to me. -Ack

I've just learned by watching the lists over the last several years. Well, and actually writing the odd patch here and there.

It occurs to me that I have been focusing on the code and not the
changelog. I'll send a seperate reply on that thread shortly.

One more thing I'll state here ... as per the perfect patch guidelines, it is preferred that the meta-discussion about the patch and its history go after the change log, seperated from it by a line of "--- " so that the patch application scripts can just extract the email subject as the title and through the firsst line of --- as the commit log. (This saves some manual editing).

[1] http://kernelnewbies.org/UpstreamMerge

milton

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Milton Miller

On Jun 5, 2007, at 8:34 AM, David Acker wrote:

David, Milton,

This was the last communication on-topic for the proposed changes to fix e100 on ARM. We're holding our breath here waiting for more, and would love to hear that this issue and fixes hasn't died off.

Thanks,

Auke

Quoted 97 lines

Quoted 8 lines

Quoted 4 lines · Milton Miller

Quoted 3 lines · David Acker

Ok, I took a stab at coding and testing these ideas. Below is a patch against 2.6.22-rc3.
Let me know what you think.

Quoted 6 lines

Quoted 3 lines

I think you got most of the ideas. As Auke noted, your coding
style is showing again. And your mailer again munged whitespace
(fixed by s/^<space><space>/<space>/ s/^$/<space>/).

Sorry about the coding style. I instinctively followed what was there instead of kernel coding convention. I will look into how whitespace is getting screwed up.

I have to watch my coding style too (I like to indent the closing brace).

At least the white space damage seems to be reversable. More than I can say for this mailer.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Find a buffer that is complete with rx->el not set and rx->s0 set.
It appears that hardware can read the rfd's el-bit, then
software can clear the rfd el-bit and set the rfd size, and then hardware can come in and read the size.

Yes, since the size is after the EL flag in the descriptor, this can happen since the pci read is not atomic.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I am reading the status back, although I don't think that I have to in this instance.

Actually, you are reading it when the rfd still has EL set. Since the cpu will never encounter that case, the if condition is never satisfied.

In my tests, every time I found a completed rfd with the el-bit set, the receiver was in the out of resources state.

Yes, if the EL was set, it would be a real hard race to find the
completed packet with EL but not RNR. I was trying to refer to where
you find a completed packet and then check for EL in the RFD. That is what I was claiming can not be observed by the cpu (unless the card writes the EL bit back, and not just the status u16).

If the unless ... above is true, then please put a comment that the device can write RFD->EL back to 1 if we raced.

Quoted 6 lines

Quoted 6 lines

How about creating a state unknown, for when we think we should check the device if its running.
If we are in this state and then encounter a received packet without s0 set, we can set it back
to running. We set it when we rx a packet with s0 set.
We then move both io_status reads to the caller.

Quoted 7 lines

I can look into that as I clean this up.

Quoted 2 lines

Quoted 2 lines

I am testing a version of this code patched against 2.6.18.4 on my PXA 255 based system. I will let you all know how it goes.

The testing I did so far did well. I will try to get some more going tonight, hopefully on a cleaned up patch.

Good to hear our expectiations match reality.

Quoted 4 lines

Quoted 3 lines

I'm assuming this is why the cleanup of the receiver start to always start on rx_to_clean got dropped again. :-)

Yep. I will get that in the next patch.

Ok.

Quoted 10 lines

Quoted 10 lines

Also, I would like a few sentences in the Driver Operation section IV Receive big comment. Something like
In order to keep updates to the RFD link field from colliding with hardware writes to mark packets complete, we use the feature that hardware will not write to a size 0 descriptor and mark the previous
packet as end-of-list (EL). After updating the link, we remove EL
and only then restore the size such that hardware may use the previous-to-end RFD.
at the end of the first paragraph, and insert software before "no locking is required" in the second.

Quoted 6 lines

Sounds good to me.

I will see if I can get into a cleaned up patch today and get it out by tomorrow. Thanks for dealing with me...I have been around kernel code for awhile but posting official patches to linux is new to me. -Ack

I've just learned by watching the lists over the last several years. Well, and actually writing the odd patch here and there.

It occurs to me that I have been focusing on the code and not the
changelog. I'll send a seperate reply on that thread shortly.

One more thing I'll state here ... as per the perfect patch guidelines, it is preferred that the meta-discussion about the patch and its history go after the change log, seperated from it by a line of "--- " so that the patch application scripts can just extract the email subject as the title and through the firsst line of --- as the commit log. (This saves some manual editing).

Quoted 8 lines

[1] http://kernelnewbies.org/UpstreamMerge

milton

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On the ARM, their is a race condition between software allocating a new receive

buffer and hardware writing into a buffer. The two race on touching the last
Receive Frame Descriptor (RFD). It has its el-bit set and its next link equal
to 0. When hardware encounters this buffer it attempts to write data to it

and then update Status Word bits and Actual Count in the RFD. At the same time
software may try to clear the el-bit and set the link address to a new buffer.
Since the entire RFD is once cache-line, the two write operations can collide. This can lead to the receive unit stalling or freed receive buffers
getting written to.

The fix is to set the el-bit on and the size to 0 on the next to last buffer
in the chain. When the hardware encounters this buffer it stops and does
not write to it at all. The hardware issues an RNR interrupt with the receive unit in the No Resources state. When software allocates buffers,
it can update the tail of the list when either it knows the hardware has stopped or the previous to the new one to mark marked.

Once it has a new next to last buffer prepared, it can clear the el-bit and set the size on the previous one. The race on this buffer is safe since the link already points to a valid next buffer.

We keep flags
in our software descriptor to note if the el bit is set and if the size was 0. When we clear the RFD's el bit and set its size, we also clear the el flag but we leave the size was 0 bit set. This was we can find this buffer again later.

If the hardware sees the el-bit cleared without the size set, it will move on to the next buffer and skip this one. If it sees the size set but the el-bit still set, it will complete that buffer and then RNR interrupt and wait.

First, a question especially to Auke and Jeff:

Since this patch both reverts the broken change that is currently in -rc and creates the fixed driver, I'm not sure I like the subject stating "on ARM", although that is the feature of the rewrite, and was the intent of merging the previous patch. This is actually its a fix for all systems relative to current, including those where dma is not cache coherent, (unlike a simple revert).

Should we just put a comment about reverting the previous patch early in the change log?

Something like this:

Fix the e100 receiver handling, supporting cache incoherent DMA.

Discard the concept of setting the S (suspend) bit with the EL bit introduced in commit d52df4a35af569071fda3f4eb08e47cc7023f094. In addition to it not setting either bit, the hardware doesn't work that way.

Thoughts?

Here is the changelog portion of the latest patch (quoted), with my comments thrown in:

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

Quoted 4 lines · Auke

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

I certainly agree with this assessment...

I've been wondering if, based on all this recent work, we should revert the s-bit stuff and wait for 2.6.23.

Quoted 1 line · Auke

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

I certainly agree with this assessment...

I've been wondering if, based on all this recent work, we should revert the s-bit stuff and wait for 2.6.23.

Quoted 5 lines · Jeff Garzik

Quoted 1 line · Auke

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

I certainly agree with this assessment...
I've been wondering if, based on all this recent work, we should revert the s-bit stuff and wait for 2.6.23.

Yes, that's my point. If Milton and David agree I think we should do so immediately.

If so, do you want me to write a revert-patch or do you have some magic to do that for me?

Quoted 7 lines · Auke

Quoted 6 lines · Jeff Garzik

Quoted 5 lines · Auke

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

Quoted 6 lines

Quoted 4 lines

I certainly agree with this assessment...
I've been wondering if, based on all this recent work, we should revert
the s-bit stuff and wait for 2.6.23.

Yes, that's my point. If Milton and David agree I think we should do so immediately.

We definitely need something other than what is in now.

Quoted 3 lines

If so, do you want me to write a revert-patch or do you have some magic to do that for me?

The simple git revert won't work because there have been other changes (ioread for instance) that conflict.

Jeff, please git-revert d52df4a35af569071fda3f4eb08e47cc7023f094 to revert the following patch for now:

Quoted 15 lines · Milton Miller

Quoted 7 lines · Auke

Quoted 6 lines · Jeff Garzik

Quoted 5 lines · Auke

We need to make sure that now that we're getting closer to 2.6.22 we don't end up killing e100 in it. Should we drop the current fixes in it to be on the safe side and aim for 2.6.23? I would hate to see an untested codepath breaking e100 on something like ppc or mips... that will be very painful

Quoted 6 lines

Quoted 4 lines

I certainly agree with this assessment...
I've been wondering if, based on all this recent work, we should revert
the s-bit stuff and wait for 2.6.23.

Yes, that's my point. If Milton and David agree I think we should do so immediately.

Quoted 8 lines

We definitely need something other than what is in now.

Quoted 3 lines

If so, do you want me to write a revert-patch or do you have some magic to do that for me?

The simple git revert won't work because there have been other changes (ioread for instance) that conflict.

Hmm git-revert seems to do the job right. I checked it with git-show | patch -p1 -R and the results look OK. The two patches on top of the one we want to revert are unrelated enough to apply (manually it shows some fuzz, but otherwise it's OK).

Jeff, please git-revert d52df4a35af569071fda3f4eb08e47cc7023f094 to revert the following patch for now:

---
commit d52df4a35af569071fda3f4eb08e47cc7023f094

Author: Scott Feldman <[email protected]>
Date:   Wed Nov 9 02:18:52 2005 -0500

[netdrvr e100] experiment with doing RX in a similar manner to eepro100

I was going to say that eepro100's speedo_rx_link() does the same DMA abuse as e100, but then I noticed one little detail: eepro100 sets both EL (end of list) and S (suspend) bits in the RFD as it chains it to the RFD list. e100 was only setting the EL bit. Hmmm, that's interesting. That means that if HW reads a RFD with the S-bit set, it'll process that RFD and then suspend the receive unit. The receive unit will resume when SW clears the S-bit. There is no need for SW to restart the receive unit. Which means a lot of the receive unit state tracking code in the driver goes away.

So here's a patch against 2.6.14. (Sorry for inlining it; the mailer I'm using now will mess with the word wrap). I can't test this on XScale (unless someone has an e100 module for Gumstix :) . It should be doing exactly what eepro100 does with RFDs. I don't believe this change will introduce a performance hit because the S-bit and EL-bit go hand-in-hand meaning if we're going to suspend because of the S- bit, we're on the last resource anyway, so we'll have to wait for SW to replenish.
(cherry picked from 29e79da9495261119e3b2e4e7c72507348e75976 commit)
---

Quoted 40 lines · Auke

Quoted 1 line

Hmm git-revert seems to do the job right. I checked it with git-show | patch -p1 -R and the results look OK. The two patches on top of the one we want to revert are unrelated enough to apply (manually it shows some fuzz, but otherwise it's OK). Jeff, please git-revert d52df4a35af569071fda3f4eb08e47cc7023f094 to revert the following patch for now:
---
commit d52df4a35af569071fda3f4eb08e47cc7023f094

Author: Scott Feldman <[email protected]>
Date:   Wed Nov 9 02:18:52 2005 -0500

[netdrvr e100] experiment with doing RX in a similar manner to
eepro100
I was going to say that eepro100's speedo_rx_link() does the
same DMA
abuse as e100, but then I noticed one little detail: eepro100
sets both
EL (end of list) and S (suspend) bits in the RFD as it chains it
to the
RFD list. e100 was only setting the EL bit. Hmmm, that's
interesting.
That means that if HW reads a RFD with the S-bit set, it'll
process
that RFD and then suspend the receive unit. The receive unit
will
resume when SW clears the S-bit. There is no need for SW to
restart
the receive unit. Which means a lot of the receive unit state
tracking
code in the driver goes away.
So here's a patch against 2.6.14. (Sorry for inlining it; the
mailer
I'm using now will mess with the word wrap). I can't test this
on
XScale (unless someone has an e100 module for Gumstix :) . It
should
be doing exactly what eepro100 does with RFDs. I don't believe
this
change will introduce a performance hit because the S-bit and
EL-bit go
hand-in-hand meaning if we're going to suspend because of the S-
bit,
we're on the last resource anyway, so we'll have to wait for SW
to

Quoted 4 lines

     replenish.
     (cherry picked from 29e79da9495261119e3b2e4e7c72507348e75976 

commit)
---

A little bit more is needed to explain why we're reverting it for now. Jeff, please insert this into the revert commit.

Auke

--
This patch attempted to fix e100 for non-cache coherent memory architectures by using the cb style code that eepro100 had and using the EL and s bits from the RFD list. Unfortunately the hardware doesn't work exactly like this and therefore this patch actually breaks e100 on those systems.

Reverting the change brings it back to the previously known good state for 2.6.22. The pending rewrite in progress to this code can then be safely merged later.

Signed-off-by Auke Kok <[email protected]>

---

Quoted 46 lines · Auke

Quoted 44 lines · Auke

Quoted 1 line

Hmm git-revert seems to do the job right. I checked it with git-show | patch -p1 -R and the results look OK. The two patches on top of the one we want to revert are unrelated enough to apply (manually it shows some fuzz, but otherwise it's OK). Jeff, please git-revert d52df4a35af569071fda3f4eb08e47cc7023f094 to revert the following patch for now:
---
commit d52df4a35af569071fda3f4eb08e47cc7023f094

Author: Scott Feldman <[email protected]>
Date:   Wed Nov 9 02:18:52 2005 -0500

[netdrvr e100] experiment with doing RX in a similar manner to
eepro100
I was going to say that eepro100's speedo_rx_link() does the
same DMA
abuse as e100, but then I noticed one little detail: eepro100
sets both
EL (end of list) and S (suspend) bits in the RFD as it chains
it to the
RFD list. e100 was only setting the EL bit. Hmmm, that's
interesting.
That means that if HW reads a RFD with the S-bit set, it'll
process
that RFD and then suspend the receive unit. The receive unit
will
resume when SW clears the S-bit. There is no need for SW to
restart
the receive unit. Which means a lot of the receive unit state
tracking
code in the driver goes away.
So here's a patch against 2.6.14. (Sorry for inlining it; the
mailer
I'm using now will mess with the word wrap). I can't test this
on
XScale (unless someone has an e100 module for Gumstix :) . It
should
be doing exactly what eepro100 does with RFDs. I don't believe
this
change will introduce a performance hit because the S-bit and
EL-bit go
hand-in-hand meaning if we're going to suspend because of the
S- bit,
we're on the last resource anyway, so we'll have to wait for SW
to

     replenish.
     (cherry picked from 29e79da9495261119e3b2e4e7c72507348e75976 

commit)
---

Quoted 6 lines

A little bit more is needed to explain why we're reverting it for now. Jeff, please insert this into the revert commit.

Auke

--
This patch attempted to fix e100 for non-cache coherent memory architectures by using the cb style code that eepro100 had and using the EL and s bits from the RFD list. Unfortunately the hardware doesn't work exactly like this and therefore this patch actually breaks e100 on those systems.

on all systems. (Both the &| typo and the removed restart logic).

Quoted 4 lines

Reverting the change brings it back to the previously known good state for 2.6.22. The pending rewrite in progress to this code can then be safely merged later.

Signed-off-by Auke Kok <[email protected]>

Quoted 70 lines · Milton Miller

Quoted 49 lines · Auke

Quoted 43 lines · Auke

Quoted 1 line

Hmm git-revert seems to do the job right. I checked it with git-show | patch -p1 -R and the results look OK. The two patches on top of the one we want to revert are unrelated enough to apply (manually it shows some fuzz, but otherwise it's OK).
Jeff, please git-revert d52df4a35af569071fda3f4eb08e47cc7023f094 to revert the following patch for now:
---
commit d52df4a35af569071fda3f4eb08e47cc7023f094

Author: Scott Feldman <[email protected]>
Date:   Wed Nov 9 02:18:52 2005 -0500

[netdrvr e100] experiment with doing RX in a similar manner to
eepro100
I was going to say that eepro100's speedo_rx_link() does the
same DMA
abuse as e100, but then I noticed one little detail: eepro100
sets both
EL (end of list) and S (suspend) bits in the RFD as it chains
it to the
RFD list. e100 was only setting the EL bit. Hmmm, that's
interesting.
That means that if HW reads a RFD with the S-bit set, it'll
process
that RFD and then suspend the receive unit. The receive unit
will
resume when SW clears the S-bit. There is no need for SW to
restart
the receive unit. Which means a lot of the receive unit state
tracking
code in the driver goes away.
So here's a patch against 2.6.14. (Sorry for inlining it; the
mailer
I'm using now will mess with the word wrap). I can't test this on XScale (unless someone has an e100 module for Gumstix :) . It
should
be doing exactly what eepro100 does with RFDs. I don't believe
this
change will introduce a performance hit because the S-bit and
EL-bit go
hand-in-hand meaning if we're going to suspend because of the
S- bit,
we're on the last resource anyway, so we'll have to wait for SW
to

Quoted 4 lines

     replenish.
     (cherry picked from 29e79da9495261119e3b2e4e7c72507348e75976 

commit)
---

Quoted 11 lines

A little bit more is needed to explain why we're reverting it for now. Jeff, please insert this into the revert commit.

Auke

Signature · 5 lines

This patch attempted to fix e100 for non-cache coherent memory 
architectures by using the cb style code that eepro100 had and using 
the EL and s bits from the RFD list. Unfortunately the hardware 
doesn't work exactly like this and therefore this patch actually 
breaks e100 on those systems.

on all systems. (Both the &| typo and the removed restart logic).

Quoted 4 lines

Reverting the change brings it back to the previously known good state for 2.6.22. The pending rewrite in progress to this code can then be safely merged later.

Quoted 2 lines

Quoted 2 lines

Signed-off-by Auke Kok <[email protected]>

Jeff I don't see this in netdev upstream-fixes. Are you waiting on Auke to respond to this?

Quoted 13 lines · David Acker

Quoted 4 lines · Milton Miller

My current reading of the manual is that the C bit will not be set on an RFD that is size 0. It goes on to processes EL and S, and decides to stop and interrupt RNR or suspend, or just go to the next packet.

I double checked this with a quick experiment and it appears you are correct.

What about if we always did the following:
set the size:
sync();
clear el-bit
sync()

Quoted 2 lines

Then if the hardware sees just the size set, the packet completes but with the el-bit and we know we need to restart since it completed. If it sees the size == 0, and the el bit set, it stops and RNR interrupts.

I think this is exposed to a hole and a race: we don't know if the hardware read the RFD before we set the size or after, just that it was before the EL bit was cleared. If it read it before the size was set, then it will not set the C bit. If it reads it after the size is set, it will complete it.

For coherent DMA we can always observe the C bit. But for the incoherent DMA case, our store to clear the EL bit may overwrite the dma from the device to the beginning of the packet, or the write to EOF, F, and size, and/or the write to C, OK, and status bits to tell us its done. In the worst case, we would overwrite the beginning of the data but catch the C bit and even the actual size, and therefore would receive corrupted data.

We can only detect the hardware went RNR when it does so or decide we won the race when it receives and completes the next frame.

Quoted 3 lines

When we find a buffer that is not completed but has the el-bit set, we read the status byte of the status control block to see if the RU is in the no resources state. If it isn't, it means that we found that buffer before the hardware did and thus need to wait for it. We will either find it on the next poll or enable interrupts and get told about it by hardware.

What do you think?

I think the second part is good ...

Quoted 2 lines

What if we just fixed the alignment of rfd to make sure it is cache aligned? Then we know our syncs will be a single cache flush.

That doesn't help in the coherent dma case; the device can still read in between, and we need to handle the !EL size 0 case.

Actually, I thought about it some more after yesterdays note. We can set the size !0 after making sure the device sees EL (wmb and pci_sync_for_device), as long as we remember this buffer used to be size 0 when doing RX clean, and look ahead for another complete packet as long as the size was ever 0.

My proposal was just to use "size is still 0" as that flag, wasting the space allocated for that skb.

Another aproach is to always allocate a rfd-only packet at the beginning of the loop, and inserting th

In commit d52df4a35af569071fda3f4eb08e47cc7023f094, the description talks about emulating another driver by setting addtional bits and the being unable to test when submitted. Seeing the & operator to set more bits made me suspicious, and indeed the bits are defined in positive logic:

	cb_s      = 0x4000,
	cb_el     = 0x8000,

So anding those together would be 0. I'm guessing they should
be or'd, but don't have hardware here to test, much like the committed patch. In fact, I'll let someone else do the compile test too. I'll update the comment.

(It looks like the end of list and s bits would not be set
in the template nor cleared when linking in recieve skbs, so
as long as the kernel can keep up with the 100Mb card we wouldn't see the adapter go off the linked list, possibly explaining any successful use of this patch written against
2.6.14).

Signed-off-by Milton Miller <[email protected]>

drivers/net/e100.c renamed from drivers/net/e100.c.orig +3 -3

@@ -947,7 +947,7 @@ static void e100_get_defaults(struct nic
 		((nic->mac >= mac_82558_D101_A4) ? cb_cid : cb_i));
 
 	/* Template for a freshly allocated RFD */
-	nic->blank_rfd.command = cpu_to_le16(cb_el & cb_s);
+	nic->blank_rfd.command = cpu_to_le16(cb_el | cb_s);
 	nic->blank_rfd.rbd = 0xFFFFFFFF;
 	nic->blank_rfd.size = cpu_to_le16(VLAN_ETH_FRAME_LEN);
 

@@ -1769,13 +1769,13 @@ static int e100_rx_alloc_skb(struct nic 
 	}
 
 	/* Link the RFD to end of RFA by linking previous RFD to
-	 * this one, and clearing EL bit of previous.  */
+	 * this one, and clearing EL and S bit of previous.  */
 	if(rx->prev->skb) {
 		struct rfd *prev_rfd = (struct rfd *)rx->prev->skb->data;
 		put_unaligned(cpu_to_le32(rx->dma_addr),
 			(u32 *)&prev_rfd->link);
 		wmb();
-		prev_rfd->command &= ~cpu_to_le16(cb_el & cb_s);
+		prev_rfd->command &= ~cpu_to_le16(cb_el | cb_s);
 		pci_dma_sync_single_for_device(nic->pdev, rx->prev->dma_addr,
 			sizeof(struct rfd), PCI_DMA_TODEVICE);
 	}