On Sun, 5 Nov 2006 14:17:38 +0100 "Eric Lemoine" <[EMAIL PROTECTED]> wrote:
> On 11/5/06, Benjamin Herrenschmidt <[EMAIL PROTECTED]> wrote: > > On Sun, 2006-11-05 at 14:00 +0100, Eric Lemoine wrote: > > > Hi! > > > > > > Some (long) time ago benh wrote a blaming comment in sungem.c about > > > that driver's locking strategy. That comment basically says that we > > > probably don't need two spinlocks. > > > > Yeah :) Note that I mostly blamed myself there ... Just never found the > > time to sit down a figure out a proper locking. > > I actually did introduce tx_lock! So you could well have blamed me :-) > > > > > > > I agree! > > > > > > Proposal: > > > > > > Today's sungem effectively uses two spinlock's: "lock" and "tx_lock". > > > > > > "tx_lock" is held by the xmit function when sending out a packet. Lots > > > of functions grab "tx_lock" not to mess up with xmit (gem_stop_phy(), > > > gem_change_mtu(), etc.). > > > > > > All of these funcs also take "lock"! > > > > > > What we could do is remove "lx_lock", have the above functions take > > > only "lock", and rely on dev->_xmit_lock to protect the xmit func from > > > reentrance. In that case, obviously, the driver wouldn't feature LLTX > > > anymore. > > > > We could probably do even better but yeah, a single lock is a good > > start. Overall, I'm unhappy with the infrastructure provided by the > > network stack though. (Might be better nowadays, but last I looked, for > > example, I couldn't properly do things like stopping MAPI poll from > > set_multicast etc... due to locks held by the upper level). > > What you said in your comment is that set_multicast and change_mtu > cannot schedule() because the upper layer holds a spinlock. This is > still the case actually. > > > > > > When (re-)configuring we'd now quiesce the device, with the new > > > functions gem_netif_stop() and gem_full_lock(), in the same way as tg3 > > > does. > > > > > > gem_interrupt(), gem_poll(), and gem_start_xmit() could become lockless. > > > Fast! > > > > > > Basically this proposal makes the data path faster, the control path > > > slower, and simplifies the code by using one single spinlock within > > > the driver. > > > > > > If the idea seems reasonable to you guys I can go ahead and cook up > > > something... > > > > I certainly does. Bring on the patch ! :-) > > Will arrange some time to do it. > > Thanks for your quick response. > > You could also just use net_tx_lock() now. - 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
