Messages in this thread | | | Date | Sat, 6 Mar 1999 16:50:21 +0100 | From | Andi Kleen <> | Subject | Re: select()/socket has problems under 2.2.x. |
| |
On Sat, Mar 06, 1999 at 03:07:15PM +0100, Andrea Arcangeli wrote: > On Sat, 6 Mar 1999, Andi Kleen wrote: > > >> - if (skb_peek(&tp->out_of_order_queue) == NULL) { > >> + if (!skb_queue_len(&tp->out_of_order_queue)) { > > > >Trivial micro optimization that is not suitable for a stable kernel patch. > > ?? > > >Or what exactly do you want to archive with this patch, other than to > >remove one jump? (which itself is fine, but for 2.3, not 2.2) > > If the skbuff-queue code is buggy you would have just noticed it in many > other part of the network code, no? I really don't see how this can be not > completly safe. > > The reason that it's been used skb_peek == NULL instead of !skb_queue_len > is only because who wrote it didn't thought that skb_peek does something > more than only returning you if the queue is empty or not.
So? And what does it more? They are equivalent, just that skb_queue_len is faster (does not jump). But remember that 2.2 is critical bug fixes only, a unnecessary jump is not a critical bug (actually once egcs will use CMOV again there will be no jump anymore on x86, but skb_queue_len will be still faster)
> > > > >> } > >> } else if (TCP_SKB_CB(skb)->ack_seq == tp->snd_una && > >> (sock_rspace(sk) || > >> - (!skb_queue_len(&sk->receive_queue) && > >> - !skb_queue_len(&tp->out_of_order_queue)))) { > >> + !skb_queue_len(&sk->receive_queue))) { > >> /* Bulk data transfer: receiver */ > >> __skb_pull(skb,th->doff*4); > > > >Still the unnecessary function call? > > > >!sock_rspace(sk) is equivalent to atomic_read(&rmem_alloc) <= sk->rcvbuf, > >except that it is much slower. > > It produced cleaner code (more easy to read) according to me. But OK I > agree with you. > > >I also think the change is wrong: such a deadlock you're trying to avoid > >here is unusual. Unusual things are not handled in the fast path. If you > > I could agree to go in the slow path but it looks like the right place to > put it according to me. Every time you see that the rcvbuf is empty you > should also check that you are not going to deadlock before reject new > incoming packets.
The basic idea behind header prediction is that you only turn it on if everything is normal. If anything unnormal happens you turn header prediction off (pred_flags = 0) and let it handle in the slow path. So for me it definitely does not look like the right place for such a check.
Other than that I think doing these costly hack just to cope with too small receive buffers is a bad idea. Just make sure the receive buffer is big enough
<tongue in cheek> if the user makes it too small it is his fault ("It hurts when I do that. Then don't do that.") Also as Alexey noted a way to make the TCP stack drop all new incoming packets is sometimes useful.</>
ok, I know that 2.2 is not the right place for such experiments, best fix to me looks like to always force a reasonable minimum buffer size.
> >Better make sure that rcvbuf is never < skb->truesize (which has the > >advantage that it doesn't cost anything in critical paths too). Best would > >be to make sure that the rcvbuf of every socket is always > maxmtu_of_devices > >+ skb header size, but that is impractical. I think 4096 is good minimum value > >and the ATM and HIPPIE people can tune it. > > What if I'll change the MTU during a transer? I just thought at the way > you are proposing and it simply doesn't look like the right way to fix the > bug to me.
I do not consider it a problem that needs fixing in 2.2. For common setups (Ethernet, FDDI, ...) a minimum value of 4096 or 8192 should work reasonably, for extreme setups with big MTUs (HIPPI, IP over ATM) this could be tuned - perhaps by a sysctl.
-Andi
-- This is like TV. I don't like TV.
- 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/
| |