Compare length of req sock queue with sk_max_ack_backlog

Compare length of req sock queue with sk_max_ack_backlog

[Tonghao Zhang]

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 ?

I hope we should
1. Add a variate in sock struct, such as sk_max_syn_backlog. Then req
sock queue and accept sock queue can control their max value. When we
create the sock, we can store the tcp_max_syn_backlog value to
sk_max_syn_backlog.

2. Update the backlog, we can update it via ioctl.

Hope for your reply.

Re: Compare length of req sock queue with sk_max_ack_backlog

[Eric Dumazet]

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.

> 
> I hope we should
> 1. Add a variate in sock struct, such as sk_max_syn_backlog. Then req
> sock queue and accept sock queue can control their max value. When we
> create the sock, we can store the tcp_max_syn_backlog value to
> sk_max_syn_backlog.
> 

There is a single /proc/sys/net/ipv4/tcp_max_syn_backlog value shared
by all sockets of a given network namespace.

But you can have thousands of TCP listeners, each of them having a
distinct listen() backlog

> 2. Update the backlog, we can update it via ioctl.

No need to add an ugly ioctl.

Simply call listen() again with another backlog value.

Re: Compare length of req sock queue with sk_max_ack_backlog

[Tonghao Zhang]

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
        }


>> I hope we should
>> 1. Add a variate in sock struct, such as sk_max_syn_backlog. Then req
>> sock queue and accept sock queue can control their max value. When we
>> create the sock, we can store the tcp_max_syn_backlog value to
>> sk_max_syn_backlog.
>>
>
> There is a single /proc/sys/net/ipv4/tcp_max_syn_backlog value shared
> by all sockets of a given network namespace.
>
> But you can have thousands of TCP listeners, each of them having a
> distinct listen() backlog
>
>> 2. Update the backlog, we can update it via ioctl.
>
> No need to add an ugly ioctl.
>
> Simply call listen() again with another backlog value.
>
>

Re: Compare length of req sock queue with sk_max_ack_backlog

[Eric Dumazet]

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.

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.

Re: Compare length of req sock queue with sk_max_ack_backlog

[Tonghao Zhang]

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.