[PATCH] tg3 locking update (with NAPI overtones)

The attached patch is based on tg3.c as found in 2.4.20-rc3 or the 
latest Linus 2.5.x kernel, and originates from two motivations:

* When net drivers move TX completion from interrupt to dev->poll(), 
this allows the rethinking of some traditional locking, namely 
eliminating a lock in dev->start_xmit() that most drivers implement 
these days.

* Specific to the tg3 implementation, spin-lock-irq is held during NIC 
hardware halt and re-initialization (and a few other operations).  In 
general a goal is to change this; the driver may wind up holding 
interrupts disabled for longer than prudence dictates.  The attached 
patch does not address this issue directly, but it does begin to lay the 
groundwork for my upcoming solution :)

I mention "NAPI" in the subject because other NAPI drivers may wish to 
re-examine their locking as well, and this patch could serve as a basis 
for discussion.


Overall, I believe the attached patch (after testing/debugging/review) 
will make the tg3 driver faster due to less locking in dev->start_xmit, 
and more friendly system interaction because interrupts are being 
disabled->enabled less frequently by this driver.


Here is a function-by-function description of changes, which gives one a 
better idea of the locking changes that may be a found in a driver:

all functions:  s/spin_lock_irq/spin_lock_irqsave/ to be more 
conservative and "obviously safe"

all functions:  locking ordering now reversed:  dev->xmit_lock obtained 
before tp->lock.

tg3_set_power_state: Do not call tg3_halt(), the one case where this 
code path is hit, the caller has already called tg3_halt()

tg3_poll: hold dev->xmit_lock when checking for TX work, and running TX 
completion cycle.  tp->tx_lock GC'd.

tg3_tx_timeout: net stack already holds dev->xmit_lock.  tx_lock GC'd.

tg3_{4gbug}_start_xmit: net stack already holds dev->xmit_lock, no other 
locks needed (whee!), so:  tp->lock GC'd.

tg3_change_mtu: get dev->xmit_lock instead of tp->tx_lock. wrap locking 
in netif_start_queue ... netif_wake_queue.

tg3_timer: get xmit_lock instead of tx_lock.  wrap NIC reset in 
netif_start_queue ... netif_wake_queue.

tg3_open: no need for tx locking, just move netif_start_queue down to 
the bottom of the function.

tg3_close: s/tp->tx_lock/dev->xmit_lock/

tg3_get_regs: likewise

tg3_ethtool_ioctl: likewise, plus netif_{start,wake}_queue wrap around 
NIC reset

tg3_vlan_rx_register: remove TX lock

tg3_vlan_rx_kill_vid: remove TX lock

tg3_suspend: s/tp->tx_lock/dev->xmit_lock/, netif_{start,wake}_queue wrap

tg3_resume: likewise

Th-th-th-th-that's all, folks!

Questions/comments/flames requested.

Looks good to me on first glance, testing will confirm
further :-)

Probably, to kill the long delays with locks held, we just
need to add a PHY config semaphore.  Interrupts that want to
try and program the PHY just do a down_trylock() on that semaphore
and defer their work if it cannot be acquired.

David S. Miller wrote:

Looks good to me on first glance, testing will confirm
further :-)

yeppers :)

Probably, to kill the long delays with locks held, we just
need to add a PHY config semaphore.  Interrupts that want to
try and program the PHY just do a down_trylock() on that semaphore
and defer their work if it cannot be acquired.

I was thinking along similar lines, and you just gave me an idea as well 
:)  Hopefully I can present a patch later on tonight showing these ideas.

For now, just responding to the above, I think there are further needs: 
  even if you acquire that semaphore, you can still wind up a full 
tg3_halt + tg3_init_hw in interrupt context, which means you could be 
there a while.  MAC/firmware init can be expensive, even ignoring the 
phy init.  Also consider the general argument that once you are 
resetting the MAC or phy, you aren't really doing useful RX/TX work; you 
can concentrate on making the slow path simple and safe while knowing 
that the fast path is idle.

So...   I prefer to simply always defer MAC/phy reset if in interrupt 
context.  This allows us to sleep as long as we want during phy and MAC 
init.  If the manual says "poll each block max of 2ms", we can do it 
easily with yield() in a loop or schedule_timeout(), the way God 
intended drivers to sleep :)

	Jeff

Just out of curiosity, have any of you been able to reproduce
any IRQ warnings under heavy loads with the tg3?  If I start
up all ports of a 4-port tulip nic and also start the tg3
(all running high speed), then I see a steady stream of
complaints from the tg3....

At least one other person has seen the same and has emailed
me about it...

-- 
Ben Greear [off-list ref]       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Ben Greear wrote:

Just out of curiosity, have any of you been able to reproduce
any IRQ warnings under heavy loads with the tg3?  If I start
up all ports of a 4-port tulip nic and also start the tg3
(all running high speed), then I see a steady stream of
complaints from the tg3....

Can you be more specific?  :)  Is it "poll already scheduled"?

Also:  is the tg3 sharing interrupts with any other device?

Jeff Garzik writes:

 > * When net drivers move TX completion from interrupt to dev->poll(), 
 > this allows the rethinking of some traditional locking, namely 
 > eliminating a lock in dev->start_xmit() that most drivers implement 
 > these days.

 Yes. 

 > 
 > tg3_tx_timeout: net stack already holds dev->xmit_lock.  tx_lock GC'd.
 
 For tx_timout and other timer/async work there may exist a scheduled poll 
 to which we have to sync. Something like this could be done. Ideas comes
 from dev->close.

 /*
  *  Synchronize and disable poll
  */

  while (test_and_set_bit(__LINK_STATE_RX_SCHED, &dev->state)) {

      	  current->state = TASK_INTERRUPTIBLE;
	  schedule_timeout(1);
  }

  .
  .

 /* Enable */
  clear_bit(__LINK_STATE_RX_SCHED, &dev->state);


 Cheers.
						--ro