Re: [PATCH] [e1000]: Remove unnecessary tx_lock

[jamal <hadi@cyberus.ca>]

On Fri, 2006-04-08 at 16:31 -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@osdl.org>

>  
> > The real risk is that the transmit routine will decide it's busy, but
> > at the same time an IRQ on another CPU will clean up the whole ring
> > and make the queue available again. Then the transmit routine exits and
> > says its busy, and the higher level will decide it is permanently stopped.
> 

Doesnt this mean we need to hold the txlock longer to avoid the race?
It will also mean we need to have a consistent programming model of the
drivers (as an example: When is it not advisable to prune tx ring on the
tx path? My gut feeling at the moment is i would rather have receive
path do a lot of dirty work so as to prioritize transmit)

> Yes, it's meant to catch unintented races.
> 

This section of the tx path is quiet hairy. Would a patch to make
qdisc_restart more readable be a sensible thing to submit?

> The queueing layer that calls ->hard_start_xmit() technically has no
> need to support NETDEV_TX_BUSY as a return value, since the device
> is able to prevent this.
> 

I noticed that the e1000 in some cases returns NETDEV_TX_OK even when it
has stopped the queue. This results in wasted calls into the qdisc.
It seems the tg3 is doing as well. I put a little counter and the amount
of those useless calls that happen in incredible.
I was going to experiment with infact returning an additional code
NETDEV_TX_BUSY_QUEUED which signals the core to just go away.

> If we could avoid NETDEV_TX_BUSY et al. from happening, the idea is
> that we could eliminate all of the requeueing logic in the packet
> scheduler layer.  It is a pipe dream from many years ago.
> 

Still doable IMO - maybe a motivation for something to experiment with
this long weekend. 


cheers,
jamal