[sungem] proposal for a new locking strategy

[sungem] proposal for a new locking strategy

[Eric Lemoine]

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.

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.

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...

Thanks,

-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Benjamin Herrenschmidt]

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 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).

> 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 ! :-)

Ben.

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

On 11/5/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.


-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

On Sun, 5 Nov 2006 14:17:38 +0100
"Eric Lemoine" <eric.lemoine@gmail.com> wrote:

> On 11/5/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

> You could also just use net_tx_lock() now.

You mean netif_tx_lock()?

Thanks for letting me know about that function. Yes, I may need it.
tg3 and bnx2 use it to wake up the transmit queue:

         if (unlikely(netif_queue_stopped(tp->dev) &&
                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
                 netif_tx_lock(tp->dev);
                 if (netif_queue_stopped(tp->dev) &&
                     (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
                         netif_wake_queue(tp->dev);
                 netif_tx_unlock(tp->dev);
         }

2.6.17 didn't use it. Was it a bug?

Thanks,
-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

On Sun, 5 Nov 2006 18:28:33 +0100
"Eric Lemoine" <eric.lemoine@gmail.com> wrote:

> > You could also just use net_tx_lock() now.
> 
> You mean netif_tx_lock()?
> 
> Thanks for letting me know about that function. Yes, I may need it.
> tg3 and bnx2 use it to wake up the transmit queue:
> 
>          if (unlikely(netif_queue_stopped(tp->dev) &&
>                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
>                  netif_tx_lock(tp->dev);
>                  if (netif_queue_stopped(tp->dev) &&
>                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
>                          netif_wake_queue(tp->dev);
>                  netif_tx_unlock(tp->dev);
>          }
> 
> 2.6.17 didn't use it. Was it a bug?
> 
> Thanks,

No, it was introduced in 2.6.18. The functions are just a wrapper
around the network device transmit lock that is normally held.

If the device does not need to acquire the lock during IRQ, it
is a good alternative and avoids a second lock.

For transmit locking there are three common alternatives:

Method A: dev->queue_xmit_lock and per-device tx_lock
	send: dev->xmit_lock held by caller
	    dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock

	irq:  netdev_priv(dev)->tx_lock acquired

Method B: dev->queue_xmit_lock only
	send: dev->xmit_lock held by caller
	irq:  schedules softirq (NAPI)
	napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock

Method C: LLTX
	set dev->features LLTX
	send: no locks held by caller
		dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
	irq: netdev_priv(dev)->tx_lock acquired

Method A is the only one that works with 2.4 and early (2.6.8?) kernels.

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> On Sun, 5 Nov 2006 18:28:33 +0100
> "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
>
> > > You could also just use net_tx_lock() now.
> >
> > You mean netif_tx_lock()?
> >
> > Thanks for letting me know about that function. Yes, I may need it.
> > tg3 and bnx2 use it to wake up the transmit queue:
> >
> >          if (unlikely(netif_queue_stopped(tp->dev) &&
> >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> >                  netif_tx_lock(tp->dev);
> >                  if (netif_queue_stopped(tp->dev) &&
> >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> >                          netif_wake_queue(tp->dev);
> >                  netif_tx_unlock(tp->dev);
> >          }
> >
> > 2.6.17 didn't use it. Was it a bug?
> >
> > Thanks,
>
> No, it was introduced in 2.6.18. The functions are just a wrapper
> around the network device transmit lock that is normally held.
>
> If the device does not need to acquire the lock during IRQ, it
> is a good alternative and avoids a second lock.
>
> For transmit locking there are three common alternatives:
>
> Method A: dev->queue_xmit_lock and per-device tx_lock
>         send: dev->xmit_lock held by caller
>             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
>
>         irq:  netdev_priv(dev)->tx_lock acquired
>
> Method B: dev->queue_xmit_lock only
>         send: dev->xmit_lock held by caller
>         irq:  schedules softirq (NAPI)
>         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
>
> Method C: LLTX
>         set dev->features LLTX
>         send: no locks held by caller
>                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
>         irq: netdev_priv(dev)->tx_lock acquired
>
> Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
>

Current sungem does Method C, and uses two locks: lock and tx_lock.
What I was planning to do is Method B (which current tg3 uses). It
seems to me that Method B is better than Method C. What do you think?

-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

On Sun, 5 Nov 2006 18:52:45 +0100
"Eric Lemoine" <eric.lemoine@gmail.com> wrote:

> On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > On Sun, 5 Nov 2006 18:28:33 +0100
> > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> >
> > > > You could also just use net_tx_lock() now.
> > >
> > > You mean netif_tx_lock()?
> > >
> > > Thanks for letting me know about that function. Yes, I may need it.
> > > tg3 and bnx2 use it to wake up the transmit queue:
> > >
> > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > >                  netif_tx_lock(tp->dev);
> > >                  if (netif_queue_stopped(tp->dev) &&
> > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > >                          netif_wake_queue(tp->dev);
> > >                  netif_tx_unlock(tp->dev);
> > >          }
> > >
> > > 2.6.17 didn't use it. Was it a bug?
> > >
> > > Thanks,
> >
> > No, it was introduced in 2.6.18. The functions are just a wrapper
> > around the network device transmit lock that is normally held.
> >
> > If the device does not need to acquire the lock during IRQ, it
> > is a good alternative and avoids a second lock.
> >
> > For transmit locking there are three common alternatives:
> >
> > Method A: dev->queue_xmit_lock and per-device tx_lock
> >         send: dev->xmit_lock held by caller
> >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> >
> >         irq:  netdev_priv(dev)->tx_lock acquired
> >
> > Method B: dev->queue_xmit_lock only
> >         send: dev->xmit_lock held by caller
> >         irq:  schedules softirq (NAPI)
> >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> >
> > Method C: LLTX
> >         set dev->features LLTX
> >         send: no locks held by caller
> >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> >         irq: netdev_priv(dev)->tx_lock acquired
> >
> > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> >
> 
> Current sungem does Method C, and uses two locks: lock and tx_lock.
> What I was planning to do is Method B (which current tg3 uses). It
> seems to me that Method B is better than Method C. What do you think?

B is better than C because the transmit logic doesn't have to
spin in the case of lock contention, but it is not a big difference.
You can only use B if you do Tx completion outside of hard IRQ.

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> On Sun, 5 Nov 2006 18:52:45 +0100
> "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
>
> > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > On Sun, 5 Nov 2006 18:28:33 +0100
> > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > >
> > > > > You could also just use net_tx_lock() now.
> > > >
> > > > You mean netif_tx_lock()?
> > > >
> > > > Thanks for letting me know about that function. Yes, I may need it.
> > > > tg3 and bnx2 use it to wake up the transmit queue:
> > > >
> > > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > > >                  netif_tx_lock(tp->dev);
> > > >                  if (netif_queue_stopped(tp->dev) &&
> > > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > > >                          netif_wake_queue(tp->dev);
> > > >                  netif_tx_unlock(tp->dev);
> > > >          }
> > > >
> > > > 2.6.17 didn't use it. Was it a bug?
> > > >
> > > > Thanks,
> > >
> > > No, it was introduced in 2.6.18. The functions are just a wrapper
> > > around the network device transmit lock that is normally held.
> > >
> > > If the device does not need to acquire the lock during IRQ, it
> > > is a good alternative and avoids a second lock.
> > >
> > > For transmit locking there are three common alternatives:
> > >
> > > Method A: dev->queue_xmit_lock and per-device tx_lock
> > >         send: dev->xmit_lock held by caller
> > >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > >
> > >         irq:  netdev_priv(dev)->tx_lock acquired
> > >
> > > Method B: dev->queue_xmit_lock only
> > >         send: dev->xmit_lock held by caller
> > >         irq:  schedules softirq (NAPI)
> > >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> > >
> > > Method C: LLTX
> > >         set dev->features LLTX
> > >         send: no locks held by caller
> > >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > >         irq: netdev_priv(dev)->tx_lock acquired
> > >
> > > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> > >
> >
> > Current sungem does Method C, and uses two locks: lock and tx_lock.
> > What I was planning to do is Method B (which current tg3 uses). It
> > seems to me that Method B is better than Method C. What do you think?
>
> B is better than C because the transmit logic doesn't have to
> spin in the case of lock contention, but it is not a big difference.

Current sungem does C but uses try_lock() to acquire its private
tx_lock. So it doesn't spin either in case of contention.

> You can only use B if you do Tx completion outside of hard IRQ.

Because qdisk_restart() is called with bh disabled *but* with irq
enabled, right?


-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

On Sun, 5 Nov 2006 21:11:34 +0100
"Eric Lemoine" <eric.lemoine@gmail.com> wrote:

> On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > On Sun, 5 Nov 2006 18:52:45 +0100
> > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> >
> > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > On Sun, 5 Nov 2006 18:28:33 +0100
> > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > >
> > > > > > You could also just use net_tx_lock() now.
> > > > >
> > > > > You mean netif_tx_lock()?
> > > > >
> > > > > Thanks for letting me know about that function. Yes, I may need it.
> > > > > tg3 and bnx2 use it to wake up the transmit queue:
> > > > >
> > > > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > > > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > > > >                  netif_tx_lock(tp->dev);
> > > > >                  if (netif_queue_stopped(tp->dev) &&
> > > > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > > > >                          netif_wake_queue(tp->dev);
> > > > >                  netif_tx_unlock(tp->dev);
> > > > >          }
> > > > >
> > > > > 2.6.17 didn't use it. Was it a bug?
> > > > >
> > > > > Thanks,
> > > >
> > > > No, it was introduced in 2.6.18. The functions are just a wrapper
> > > > around the network device transmit lock that is normally held.
> > > >
> > > > If the device does not need to acquire the lock during IRQ, it
> > > > is a good alternative and avoids a second lock.
> > > >
> > > > For transmit locking there are three common alternatives:
> > > >
> > > > Method A: dev->queue_xmit_lock and per-device tx_lock
> > > >         send: dev->xmit_lock held by caller
> > > >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > >
> > > >         irq:  netdev_priv(dev)->tx_lock acquired
> > > >
> > > > Method B: dev->queue_xmit_lock only
> > > >         send: dev->xmit_lock held by caller
> > > >         irq:  schedules softirq (NAPI)
> > > >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> > > >
> > > > Method C: LLTX
> > > >         set dev->features LLTX
> > > >         send: no locks held by caller
> > > >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > >         irq: netdev_priv(dev)->tx_lock acquired
> > > >
> > > > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> > > >
> > >
> > > Current sungem does Method C, and uses two locks: lock and tx_lock.
> > > What I was planning to do is Method B (which current tg3 uses). It
> > > seems to me that Method B is better than Method C. What do you think?
> >
> > B is better than C because the transmit logic doesn't have to
> > spin in the case of lock contention, but it is not a big difference.
> 
> Current sungem does C but uses try_lock() to acquire its private
> tx_lock. So it doesn't spin either in case of contention.


But the spin is still there, just more complex..
In qdisc_restart() processing of NETDEV_TX_LOCKED causes:
	spin_lock(dev->xmit_lock)
	
	q->requeue()
	netif_schedule(dev);

SOFTIRQ:
	net_tx_action()
	qdisc_run() --> qdisc_restart()

So instead of spinning in tight loop, you end up with a longer code
path.


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

On 11/6/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> On Sun, 5 Nov 2006 21:11:34 +0100
> "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
>
> > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > On Sun, 5 Nov 2006 18:52:45 +0100
> > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > >
> > > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > > On Sun, 5 Nov 2006 18:28:33 +0100
> > > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > > >
> > > > > > > You could also just use net_tx_lock() now.
> > > > > >
> > > > > > You mean netif_tx_lock()?
> > > > > >
> > > > > > Thanks for letting me know about that function. Yes, I may need it.
> > > > > > tg3 and bnx2 use it to wake up the transmit queue:
> > > > > >
> > > > > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > > > > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > > > > >                  netif_tx_lock(tp->dev);
> > > > > >                  if (netif_queue_stopped(tp->dev) &&
> > > > > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > > > > >                          netif_wake_queue(tp->dev);
> > > > > >                  netif_tx_unlock(tp->dev);
> > > > > >          }
> > > > > >
> > > > > > 2.6.17 didn't use it. Was it a bug?
> > > > > >
> > > > > > Thanks,
> > > > >
> > > > > No, it was introduced in 2.6.18. The functions are just a wrapper
> > > > > around the network device transmit lock that is normally held.
> > > > >
> > > > > If the device does not need to acquire the lock during IRQ, it
> > > > > is a good alternative and avoids a second lock.
> > > > >
> > > > > For transmit locking there are three common alternatives:
> > > > >
> > > > > Method A: dev->queue_xmit_lock and per-device tx_lock
> > > > >         send: dev->xmit_lock held by caller
> > > > >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > >
> > > > >         irq:  netdev_priv(dev)->tx_lock acquired
> > > > >
> > > > > Method B: dev->queue_xmit_lock only
> > > > >         send: dev->xmit_lock held by caller
> > > > >         irq:  schedules softirq (NAPI)
> > > > >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> > > > >
> > > > > Method C: LLTX
> > > > >         set dev->features LLTX
> > > > >         send: no locks held by caller
> > > > >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > >         irq: netdev_priv(dev)->tx_lock acquired
> > > > >
> > > > > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> > > > >
> > > >
> > > > Current sungem does Method C, and uses two locks: lock and tx_lock.
> > > > What I was planning to do is Method B (which current tg3 uses). It
> > > > seems to me that Method B is better than Method C. What do you think?
> > >
> > > B is better than C because the transmit logic doesn't have to
> > > spin in the case of lock contention, but it is not a big difference.
> >
> > Current sungem does C but uses try_lock() to acquire its private
> > tx_lock. So it doesn't spin either in case of contention.
>
>
> But the spin is still there, just more complex..
> In qdisc_restart() processing of NETDEV_TX_LOCKED causes:
>         spin_lock(dev->xmit_lock)
>
>         q->requeue()
>         netif_schedule(dev);
>
> SOFTIRQ:
>         net_tx_action()
>         qdisc_run() --> qdisc_restart()
>
> So instead of spinning in tight loop, you end up with a longer code
> path.

Stephen, sorry for insisting a bit but I'm failing to see how B is
different from C in that respect. With method B, in qdisc_restart(),
if netif_tx_trylock() fails to acquire the lock then we also
requeue(), etc. Same long code path in case of contention.

-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

On Mon, 6 Nov 2006 21:55:20 +0100
"Eric Lemoine" <eric.lemoine@gmail.com> wrote:

> On 11/6/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > On Sun, 5 Nov 2006 21:11:34 +0100
> > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> >
> > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > On Sun, 5 Nov 2006 18:52:45 +0100
> > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > >
> > > > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > > > On Sun, 5 Nov 2006 18:28:33 +0100
> > > > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > > > >
> > > > > > > > You could also just use net_tx_lock() now.
> > > > > > >
> > > > > > > You mean netif_tx_lock()?
> > > > > > >
> > > > > > > Thanks for letting me know about that function. Yes, I may need it.
> > > > > > > tg3 and bnx2 use it to wake up the transmit queue:
> > > > > > >
> > > > > > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > > > > > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > > > > > >                  netif_tx_lock(tp->dev);
> > > > > > >                  if (netif_queue_stopped(tp->dev) &&
> > > > > > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > > > > > >                          netif_wake_queue(tp->dev);
> > > > > > >                  netif_tx_unlock(tp->dev);
> > > > > > >          }
> > > > > > >
> > > > > > > 2.6.17 didn't use it. Was it a bug?
> > > > > > >
> > > > > > > Thanks,
> > > > > >
> > > > > > No, it was introduced in 2.6.18. The functions are just a wrapper
> > > > > > around the network device transmit lock that is normally held.
> > > > > >
> > > > > > If the device does not need to acquire the lock during IRQ, it
> > > > > > is a good alternative and avoids a second lock.
> > > > > >
> > > > > > For transmit locking there are three common alternatives:
> > > > > >
> > > > > > Method A: dev->queue_xmit_lock and per-device tx_lock
> > > > > >         send: dev->xmit_lock held by caller
> > > > > >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > > >
> > > > > >         irq:  netdev_priv(dev)->tx_lock acquired
> > > > > >
> > > > > > Method B: dev->queue_xmit_lock only
> > > > > >         send: dev->xmit_lock held by caller
> > > > > >         irq:  schedules softirq (NAPI)
> > > > > >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> > > > > >
> > > > > > Method C: LLTX
> > > > > >         set dev->features LLTX
> > > > > >         send: no locks held by caller
> > > > > >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > > >         irq: netdev_priv(dev)->tx_lock acquired
> > > > > >
> > > > > > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> > > > > >
> > > > >
> > > > > Current sungem does Method C, and uses two locks: lock and tx_lock.
> > > > > What I was planning to do is Method B (which current tg3 uses). It
> > > > > seems to me that Method B is better than Method C. What do you think?
> > > >
> > > > B is better than C because the transmit logic doesn't have to
> > > > spin in the case of lock contention, but it is not a big difference.
> > >
> > > Current sungem does C but uses try_lock() to acquire its private
> > > tx_lock. So it doesn't spin either in case of contention.
> >
> >
> > But the spin is still there, just more complex..
> > In qdisc_restart() processing of NETDEV_TX_LOCKED causes:
> >         spin_lock(dev->xmit_lock)
> >
> >         q->requeue()
> >         netif_schedule(dev);
> >
> > SOFTIRQ:
> >         net_tx_action()
> >         qdisc_run() --> qdisc_restart()
> >
> > So instead of spinning in tight loop, you end up with a longer code
> > path.
> 
> Stephen, sorry for insisting a bit but I'm failing to see how B is
> different from C in that respect. With method B, in qdisc_restart(),
> if netif_tx_trylock() fails to acquire the lock then we also
> requeue(), etc. Same long code path in case of contention.
> 

Method C LLTX causes repeated softirq's which will be slower since the loop
requires more instructions than a simple spin loop (Method B).


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [sungem] proposal for a new locking strategy

[Eric Lemoine]

On 11/6/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> On Mon, 6 Nov 2006 21:55:20 +0100
> "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
>
> > On 11/6/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > On Sun, 5 Nov 2006 21:11:34 +0100
> > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > >
> > > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > > On Sun, 5 Nov 2006 18:52:45 +0100
> > > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > > >
> > > > > > On 11/5/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> > > > > > > On Sun, 5 Nov 2006 18:28:33 +0100
> > > > > > > "Eric Lemoine" <eric.lemoine@gmail.com> wrote:
> > > > > > >
> > > > > > > > > You could also just use net_tx_lock() now.
> > > > > > > >
> > > > > > > > You mean netif_tx_lock()?
> > > > > > > >
> > > > > > > > Thanks for letting me know about that function. Yes, I may need it.
> > > > > > > > tg3 and bnx2 use it to wake up the transmit queue:
> > > > > > > >
> > > > > > > >          if (unlikely(netif_queue_stopped(tp->dev) &&
> > > > > > > >                       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))) {
> > > > > > > >                  netif_tx_lock(tp->dev);
> > > > > > > >                  if (netif_queue_stopped(tp->dev) &&
> > > > > > > >                      (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH))
> > > > > > > >                          netif_wake_queue(tp->dev);
> > > > > > > >                  netif_tx_unlock(tp->dev);
> > > > > > > >          }
> > > > > > > >
> > > > > > > > 2.6.17 didn't use it. Was it a bug?
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > >
> > > > > > > No, it was introduced in 2.6.18. The functions are just a wrapper
> > > > > > > around the network device transmit lock that is normally held.
> > > > > > >
> > > > > > > If the device does not need to acquire the lock during IRQ, it
> > > > > > > is a good alternative and avoids a second lock.
> > > > > > >
> > > > > > > For transmit locking there are three common alternatives:
> > > > > > >
> > > > > > > Method A: dev->queue_xmit_lock and per-device tx_lock
> > > > > > >         send: dev->xmit_lock held by caller
> > > > > > >             dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > > > >
> > > > > > >         irq:  netdev_priv(dev)->tx_lock acquired
> > > > > > >
> > > > > > > Method B: dev->queue_xmit_lock only
> > > > > > >         send: dev->xmit_lock held by caller
> > > > > > >         irq:  schedules softirq (NAPI)
> > > > > > >         napi_poll: calls netif_tx_lock() which acquires dev->xmit_lock
> > > > > > >
> > > > > > > Method C: LLTX
> > > > > > >         set dev->features LLTX
> > > > > > >         send: no locks held by caller
> > > > > > >                 dev->hard_start_xmit acquires netdev_priv(dev)->tx_lock
> > > > > > >         irq: netdev_priv(dev)->tx_lock acquired
> > > > > > >
> > > > > > > Method A is the only one that works with 2.4 and early (2.6.8?) kernels.
> > > > > > >
> > > > > >
> > > > > > Current sungem does Method C, and uses two locks: lock and tx_lock.
> > > > > > What I was planning to do is Method B (which current tg3 uses). It
> > > > > > seems to me that Method B is better than Method C. What do you think?
> > > > >
> > > > > B is better than C because the transmit logic doesn't have to
> > > > > spin in the case of lock contention, but it is not a big difference.
> > > >
> > > > Current sungem does C but uses try_lock() to acquire its private
> > > > tx_lock. So it doesn't spin either in case of contention.
> > >
> > >
> > > But the spin is still there, just more complex..
> > > In qdisc_restart() processing of NETDEV_TX_LOCKED causes:
> > >         spin_lock(dev->xmit_lock)
> > >
> > >         q->requeue()
> > >         netif_schedule(dev);
> > >
> > > SOFTIRQ:
> > >         net_tx_action()
> > >         qdisc_run() --> qdisc_restart()
> > >
> > > So instead of spinning in tight loop, you end up with a longer code
> > > path.
> >
> > Stephen, sorry for insisting a bit but I'm failing to see how B is
> > different from C in that respect. With method B, in qdisc_restart(),
> > if netif_tx_trylock() fails to acquire the lock then we also
> > requeue(), etc. Same long code path in case of contention.
> >
>
> Method C LLTX causes repeated softirq's which will be slower since the loop
> requires more instructions than a simple spin loop (Method B).

What I'm saying above is that Method B also causes repeated tx
softirqs in case of contention on netif_tx_lock. The code path is :
netif_tx_trylock() fails -> requeue() -> netif_schedule() ->
raise_softirq(NET_TX_SOFTIRQ). Am I missing anything?


-- 
Eric

Re: [sungem] proposal for a new locking strategy

[Stephen Hemminger]

O
> > > > But the spin is still there, just more complex..
> > > > In qdisc_restart() processing of NETDEV_TX_LOCKED causes:
> > > >         spin_lock(dev->xmit_lock)
> > > >
> > > >         q->requeue()
> > > >         netif_schedule(dev);
> > > >
> > > > SOFTIRQ:
> > > >         net_tx_action()
> > > >         qdisc_run() --> qdisc_restart()
> > > >
> > > > So instead of spinning in tight loop, you end up with a longer code
> > > > path.
> > >
> > > Stephen, sorry for insisting a bit but I'm failing to see how B is
> > > different from C in that respect. With method B, in qdisc_restart(),
> > > if netif_tx_trylock() fails to acquire the lock then we also
> > > requeue(), etc. Same long code path in case of contention.
> > >
> >
> > Method C LLTX causes repeated softirq's which will be slower since the loop
> > requires more instructions than a simple spin loop (Method B).
> 
> What I'm saying above is that Method B also causes repeated tx
> softirqs in case of contention on netif_tx_lock. The code path is :
> netif_tx_trylock() fails -> requeue() -> netif_schedule() ->
> raise_softirq(NET_TX_SOFTIRQ). Am I missing anything?
> 

Never mind, you right.  I think the netif_tx_trylock() there is a bad
idea because of the long route as shown.  Trylock's are usually bad SMP
design anyway. Not sure if it would be possible to change the locking
in qdisc_restart() without causing collateral damage in network drivers.


-- 
Stephen Hemminger <shemminger@osdl.org>