Ok. I'll take your word for it.
> > Ergo, every single use of tbusy seems to be buggy. Better just get
> > rid of it.
>
> And later on you propose replacing it with itself but broken. Renaming it
> to avoid accidents makes sense. OK
I do propose replacing it by itself, but it's really to make sure we get
rid of the old and stale use, which at least to me looks to be the bulk of
it.
> > safe - it happens to work apparently because all drivers when they
> > change their state of tbusy will also force a NET_BH to be run,
> > even if they don't actually have any incoming packets.
>
> That is -explicitly- required by the use of tbusy and it was always designed
> like that. You have to schedule a bh on a tbusy clear otherwise nothing in
> the kernel will feed you packets. You can't change that. Your design
> below requires fixing to do this. Someone has to schedule the delivery
> of further packets. Therefore you must use mark_bh()
No, you're right. My "fix" below just needs to be fixed to also do the
NET_BH setting. I thought the network drivers pulled the send queue on
their own, but looking closer it's obvious that they don't (and that they
should not for locking issues).
> It works fine. The broken case is the fact that tbusy isnt enforced on the
> first send. To fix that requires putting in a dev->timeout handler for
> each device. If you break that behaviour then the moment your queue is full
> and you take a tx timeout you are dead.
Eww. Ok, I see why it's not set. It looks like a bad design, but it's
obvious we can't fix that for 2.2.
> > if (!test_and_set_bit(DEV_TRANSMIT, &dev->flags)) {
>
> test_and_set_bit has fairly poor performance on some non intel
> architectures because it is implemented with spin locks. We want something
> that can use things like sparc exchange with 0xFF locking.
>
> (also dev->flags is already all reserved). dev->flow ?
Sure.
> That adds potential races and makes life extremely hard. It also doesnt
> work as you need to mark_bh(NET_BH) so net_bh feeds it more packets.
I agree about NET_BH now that I looked at it some more. And if you think
clearing it unconditionally makes sense, I'll just take your word for it.
So maybe we could just to the minimal rename for 2.2.x and I certainly
agree with "dev->flow" - which makes clear what it is for. Just getting
rid of the incorrect uses would be good, you've convinced me that we can't
make it pretty.
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/