Re: select()/socket has problems under 2.2.x.

Andi Kleen (ak@muc.de)
Sat, 6 Mar 1999 13:31:35 +0100


On Fri, Mar 05, 1999 at 10:25:36PM +0100, Andrea Arcangeli wrote:
> On 5 Mar 1999, Andi Kleen wrote:
>
> >andrea@e-mind.com (Andrea Arcangeli) writes:
> >> }
> >> } else if (TCP_SKB_CB(skb)->ack_seq == tp->snd_una &&
> >> - atomic_read(&sk->rmem_alloc) <= sk->rcvbuf) {
> >> + (sock_rspace(sk) ||
> >> + (!skb_queue_len(&sk->receive_queue) &&
> >> + !skb_queue_len(&tp->out_of_order_queue)))) {
> >> /* Bulk data transfer: receiver */
> >> __skb_pull(skb,th->doff*4);
> >
> >The out_of_order_queue check is bogus because the fast path should be never
> >entered when there are ooo packets. Also you just introduced a costly
>
> Woops agreed, thanks.
>
> Here an incremental patch (I did also some performance optimization):
>
> Index: tcp_input.c
> ===================================================================
> RCS file: /var/cvs/linux/net/ipv4/tcp_input.c,v
> retrieving revision 1.1.2.19
> diff -u -r1.1.2.19 tcp_input.c
> --- tcp_input.c 1999/03/05 19:47:01 1.1.2.19
> +++ tcp_input.c 1999/03/05 21:15:58
> @@ -1421,7 +1421,7 @@
> SOCK_DEBUG(sk, "out of order segment: rcv_next %X seq %X - %X\n",
> tp->rcv_nxt, TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq);
>
> - 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)

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

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
really think it is necessary add it to the "turn off fast path" check
in tcp_data_queue - such things should be definitely handled in the slow
path. But I think it is not - see below.

>
> >I also don't see what you're trying to archieve with this change.
>
> If rcvbuf < skb->truesize you won't have way to receive something without
> my patch. My changes allow always a packet to be queued in the receiver
> and so it avoids deadlocking in the receiver.

Which allows a DOS - the receiver could overrun the socket buffer easily.
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.

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