Re: Compare length of req sock queue with sk_max_ack_backlog

[Tonghao Zhang <xiangxia.m.yue@gmail.com>]

On Fri, Feb 2, 2018 at 12:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2018-02-02 at 09:55 +0800, Tonghao Zhang wrote:
>> On Thu, Feb 1, 2018 at 10:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Thu, 2018-02-01 at 20:34 +0800, Tonghao Zhang wrote:
>> > > Hi Eric
>> > > One question for you, In the patch ef547f2ac16 ("tcp: remove
>> > > max_qlen_log"),  why we compared the length of req sock queue with
>> > > sk_max_ack_backlog. If we remove the max_qlen_log, we should check the
>> > > length of req sock queue with tcp_max_syn_backlog, right ?
>> > >
>> > > With this patch, the option "/proc/sys/net/ipv4/tcp_max_syn_backlog"
>> > > will be unsed anymore, right ?
>> >
>> > Not right.
>> >
>> > Please "git grep -n sysctl_max_syn_backlog" to convince it is still
>> > used.
>> >
>>
>> In the tcp_conn_request(), we call the  inet_csk_reqsk_queue_is_full()
>> to check the length of req sock queue. But
>> inet_csk_reqsk_queue_is_full()
>> compares it with sk_max_ack_backlog.
>>
>> inet_csk_reqsk_queue_is_full
>>          inet_csk_reqsk_queue_len(sk) >= sk->sk_max_ack_backlog;
>>
>>
>> inet_csk_reqsk_queue_len
>>          reqsk_queue_len(&inet_csk(sk)->icsk_accept_queue);
>>
>> If we set sysctl_max_syn_backlog to 1024 via
>> /proc/sys/net/ipv4/tcp_max_syn_backlog, and
>> backlog(sk_max_ack_backlog) to 128 via listen() , tcp_conn_request
>> will drop the packets when
>> length of req sock queue > 128, but not 1024.
>>
>> tcp_conn_request
>>         if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
>>              inet_csk_reqsk_queue_is_full(sk)) && !isn) {
>>                 want_cookie = tcp_syn_flood_action(sk, skb, rsk_ops->slab_name);
>>                 if (!want_cookie)
>>                         goto drop;  // drop the packets
>>         }
>
> It seems to work as intended and documented in the changelog.
>
> A socket listen backlog is determined at the time listen() system call
> is issued, using the standard somaxconn as safe guard, for all socket
> types.
>
> We want to get rid of tcp_max_syn_backlog eventually, since TCP
> listeners can use listen(fd, 10000000) these days if they want,
> it is a matter of provisioning the needed memory.

In some case, the user may use different value for tcp_max_syn_backlog
and sk_max_ack_backlog.
Then in the tcp_conn_request, we should compare the length of req sock
queue with tcp_max_syn_backlog, but not sk_max_ack_backlog.
We can update the tcp_max_syn_backlog for specific socket via ioctl,
such like listen updating the ack backlog.

inet_csk_reqsk_queue_is_full
          inet_csk_reqsk_queue_len(sk) >= sk->sk_max_ack_backlog;

to:
inet_csk_reqsk_queue_is_full
          inet_csk_reqsk_queue_len(sk) >= tcp_max_syn_backlog; // for example


The sk_acceptq_is_full is ok in the sk_acceptq_is_full().

sk_acceptq_is_full
         sk->sk_ack_backlog > sk->sk_max_ack_backlog;

> Old mechanism was not allowing a listener to reduce or increase its
> backlog dynamically (listener was using a private hash table, sized at
> the time of first listen() and not resized later)
>
> Having a global sysctl to magically change behavior on all TCP
> listeners is not very practical, unless you had dedicated hosts to deal
> with one service.
I got it.