Re: [sungem] proposal for a new locking strategy

["Eric Lemoine" <eric.lemoine@gmail.com>]

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