tg3_msi() and weakly ordered memory

Will setting during-ints to a very high threshhold essentially allow us to "indefinitely" process stuff without taking any interrupts? Would the threshhold counter get reset every time we write back the status tag WITHOUT re-enableing interrupts?

If not, I suspect the CPU will circulate in tg3_poll until during-ints is exhausted and DMA will stop until CPU reenables interrupts. ie not until it's done processing outstanding packets.

thanks,
grant

Ok, here is the patch I came up with as a result of this thread.

Michael stated he would investigate using a pure tag comparison in place of tg3_has_work() when the chip is using tagged interrupts.

Thanks.

[TG3]: Fix missing memory barriers and SD_STATUS_UPDATED bit clearing.

There must be a rmb() between reading the status block tag and calling tg3_has_work(). This was missing in tg3_mis() and tg3_interrupt_tagged(). tg3_poll() got it right.

Also, SD_STATUS_UPDATED must be cleared in the status block right before we call tg3_has_work(). Only tg3_poll() got this wrong.

Based upon patches and commentary from Grant Grundler and Michael Chan.

@@ -2929,6 +2929,7 @@
 	if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)
 		tp->last_tag = sblk->status_tag;
 	rmb();
+	sblk->status &= ~SD_STATUS_UPDATED;
 
 	/* if no more work, tell net stack and NIC we're done */
 	done = !tg3_has_work(tp);

@@ -2964,6 +2965,7 @@
 	 */
 	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
 	tp->last_tag = sblk->status_tag;
+	rmb();
 	sblk->status &= ~SD_STATUS_UPDATED;
 	if (likely(tg3_has_work(tp)))
 		netif_rx_schedule(dev);		/* schedule NAPI poll */

@@ -3051,6 +3053,7 @@
 		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     0x00000001);
 		tp->last_tag = sblk->status_tag;
+		rmb();
 		sblk->status &= ~SD_STATUS_UPDATED;
 		if (likely(tg3_has_work(tp)))
 			netif_rx_schedule(dev);		/* schedule NAPI poll */

looks good to me.

The more I think about it, the more I like the idea of each ISR calling into a different tg3_poll routine. The specific _poll() routine could do the "is there more work" checking instead the TX/RX ring cleanup code. The main reason is the "more work" checks can be better optimized for MSI (use tags) vs IRQ Line interrupt (use ring indices) handlers. I also hope to reduce cacheline movement by touching the status block fewer times.

This isn't a trivial patch and I'm short on time (preparing stuff for OLS and HP World before my vacation). If there is still interest, I can prototype a patch in late August or Sept (about 8 weeks from now).

Welcome and thanks too.

BTW, I greatly appreciate Michael clarifying tg3 behavior.

thanks,
grant

Hi David

Small patch to save an unecessary call to strlen() : sprintf() gave us the length, just trust it.

Thank you

Eric Dumazet

diff -Nu linux-2.6.12-orig/net/socket.c linux-2.6.12/net/socket.c
@@ -382,9 +382,8 @@
                        goto out;
                }
 
-               sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+               this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
                this.name = name;
-               this.len = strlen(name);
                this.hash = SOCK_INODE(sock)->i_ino;
 
                file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);

@@ -382,9 +382,8 @@
 			goto out;
 		}
 
-		sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+		this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
 		this.name = name;
-		this.len = strlen(name);
 		this.hash = SOCK_INODE(sock)->i_ino;
 
 		file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);