* netdev ioctl & dev_base_lock : bad idea ?
@ 2004-11-26 8:48 Benjamin Herrenschmidt
2004-12-09 6:06 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2004-11-26 8:48 UTC (permalink / raw)
To: netdev
Hi !
While working on simplifying sungem, I had a problem with locking.
Basically, I'm forced to do a lot of things under spinlocks, a lot more
than I should have to, because in a few places, I can't schedule. This
is typically the case of ioctl handling, and more specifically,
change_mtu() and set_multicast() callbacks.
For some reason, a while ago, those calls got a
read_lock(&dev_base_lock) added aroud them in net/core/dev.c. That means
they can't schedule, which is by itself a problem, since it force them
to use spinlocks as a synchronisation primitive and prevents them to
call netif_stop_polling(). Thus, they can't stop NAPI, which force the
napi poll() callback to take a lock too (we end up with 2 locks in there
now in sungem) while some careful coding (stopping the queue, stopping
polling, stopping chip irqs) could have permitted to not do any locking
and eventually schedule in a few places where I need to wait some time
instead of udelay.
I suppose there is a good reason we can't just use the rtnl_sem for
these guys, though why isn't dev_base_lock a read/write semaphore
instead of a spinlock ? At least on ppc, I don't think there's any
overhead in the normal path, and this is not on a very critical path
anyway, is it ?
Since we never take this lock with irq masking, I suppose there is no
problem with trying to lock at irq time, is there ? Or may we try to
acquire it occasionally from some contexts where a spinlock is already
held ?
Ben.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: netdev ioctl & dev_base_lock : bad idea ?
2004-11-26 8:48 netdev ioctl & dev_base_lock : bad idea ? Benjamin Herrenschmidt
@ 2004-12-09 6:06 ` David S. Miller
2004-12-09 6:22 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: David S. Miller @ 2004-12-09 6:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: netdev
On Fri, 26 Nov 2004 19:48:49 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> I suppose there is a good reason we can't just use the rtnl_sem for
> these guys, though why isn't dev_base_lock a read/write semaphore
> instead of a spinlock ? At least on ppc, I don't think there's any
> overhead in the normal path, and this is not on a very critical path
> anyway, is it ?
It can't be a semphore because it is taken in packet processing,
and thus softint handling, paths.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: netdev ioctl & dev_base_lock : bad idea ?
2004-12-09 6:06 ` David S. Miller
@ 2004-12-09 6:22 ` Benjamin Herrenschmidt
2004-12-09 7:13 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2004-12-09 6:22 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
On Wed, 2004-12-08 at 22:06 -0800, David S. Miller wrote:
> On Fri, 26 Nov 2004 19:48:49 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > I suppose there is a good reason we can't just use the rtnl_sem for
> > these guys, though why isn't dev_base_lock a read/write semaphore
> > instead of a spinlock ? At least on ppc, I don't think there's any
> > overhead in the normal path, and this is not on a very critical path
> > anyway, is it ?
>
> It can't be a semphore because it is taken in packet processing,
> and thus softint handling, paths.
Right, and I missed the fact that we did indeed take the semaphore and
not the lock in the _set_ functions which is just fine, we can actually
schedule.... except in set_multicast...
Is there any reason we actually _need_ to get the xmit lock in this one
specifically ?
Ben.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: netdev ioctl & dev_base_lock : bad idea ?
2004-12-09 6:22 ` Benjamin Herrenschmidt
@ 2004-12-09 7:13 ` David S. Miller
2004-12-09 22:14 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: David S. Miller @ 2004-12-09 7:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: netdev
On Thu, 09 Dec 2004 17:22:13 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> Right, and I missed the fact that we did indeed take the semaphore and
> not the lock in the _set_ functions which is just fine, we can actually
> schedule.... except in set_multicast...
>
> Is there any reason we actually _need_ to get the xmit lock in this one
> specifically ?
Since we implement NETIF_F_LLTX, the core packet transmit routines do
no locking, the driver does it all.
So if we don't hold the tx lock in the set multicast routine, any other
cpu can come into our hard_start_xmit function and poke at the hardware.
Upon further consideration, it seems that it may be OK to drop that tx
lock right after we do the netif_stop_queue(). But we should regrab
the tx lock when we do the subsequent netif_wake_queue().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: netdev ioctl & dev_base_lock : bad idea ?
2004-12-09 7:13 ` David S. Miller
@ 2004-12-09 22:14 ` Benjamin Herrenschmidt
2004-12-09 23:19 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2004-12-09 22:14 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
On Wed, 2004-12-08 at 23:13 -0800, David S. Miller wrote:
> On Thu, 09 Dec 2004 17:22:13 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > Right, and I missed the fact that we did indeed take the semaphore and
> > not the lock in the _set_ functions which is just fine, we can actually
> > schedule.... except in set_multicast...
> >
> > Is there any reason we actually _need_ to get the xmit lock in this one
> > specifically ?
>
> Since we implement NETIF_F_LLTX, the core packet transmit routines do
> no locking, the driver does it all.
>
> So if we don't hold the tx lock in the set multicast routine, any other
> cpu can come into our hard_start_xmit function and poke at the hardware.
>
> Upon further consideration, it seems that it may be OK to drop that tx
> lock right after we do the netif_stop_queue(). But we should regrab
> the tx lock when we do the subsequent netif_wake_queue().
Yes. In fact, I think it should be driver local locking policy, and not
enforced by net/core/*.
For example, for things like USB based networking (or other "remote"
busses like that), it's both very useful to be able to schedule in
set_multicast, and there is no need for any synchronisation with the
xmit code.
For things like sungem, I already have a driver local lock that can be
used if necessary.
Also, the lack of ability to schedule means we can't suspend and resume
NAPI polling, which basically forces us to take a lock in the NAPI poll
side of the driver... I'm aiming at limiting the amount of locks we take
in sungem along with moving as much as I can to task level so I can do a
bit better power management without having big u/mdelay's all over.
Also, why would we need the xmit lock when calling netif_wake_queue() ?
I'm not sure I get that one (but I'm not too familiar with the net core
neither).
Ben.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-12-09 23:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-11-26 8:48 netdev ioctl & dev_base_lock : bad idea ? Benjamin Herrenschmidt
2004-12-09 6:06 ` David S. Miller
2004-12-09 6:22 ` Benjamin Herrenschmidt
2004-12-09 7:13 ` David S. Miller
2004-12-09 22:14 ` Benjamin Herrenschmidt
2004-12-09 23:19 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).