netdev - Re: [PATCH] sctp: set ooo_okay properly for Transmit Packet Steering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFRmqq42HqX5KctcNjwyZJ4jdknLSZ1EyBqHnJQQJx211mWopw@mail.gmail.com>
Date:   Wed, 30 Oct 2019 23:54:45 +0800
From:   Wei Zhao <wallyzhao@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     vyasevich@...il.com, nhorman@...driver.com, davem@...emloft.net,
        linux-sctp@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, wally.zhao@...ia-sbell.com
Subject: Re: [PATCH] sctp: set ooo_okay properly for Transmit Packet Steering

On 10/30/19, Marcelo Ricardo Leitner <marcelo.leitner@...il.com> wrote:
> On Wed, Oct 30, 2019 at 12:07:17PM -0400, Wally Zhao wrote:
>> Unlike tcp_transmit_skb,
>> sctp_packet_transmit does not set ooo_okay explicitly,
>> causing unwanted Tx queue switching when multiqueue is in use;
>
> It is initialized to 0 by __alloc_skb() via:
>         memset(skb, 0, offsetof(struct sk_buff, tail));
> and never set to 1 by anyone for SCTP.
>
> The patch description seems off. I don't see how the unwanted Tx queue
> switching can happen. IOW, it's not fixing it OOO packets, but
> improving it by allowing switching on the first packet. Am I missing
> something?

Thanks for pointing this out. You are right. This ooo_okay is default to false.

I was observing some Tx queue switching before when testing with
iperf3 (modified to be able to set window size, for higher throughput
with long RTT), so I thought ooo_okay was set to true somewhere else
after allocation. Just now I did the test again, it turns out that
iperf3 made a re-connect silently which caused the Tx queue change.

As for the improving purpose of this patch, that is not that critical
from my side, and the patch description is not correct for this
purpose. So I will give up this patch attempt. Thank you again for
your time on this.

>
>> Tx queue switching may cause out-of-order packets.
>> Change sctp_packet_transmit to allow Tx queue switching only for
>> the first in flight packet, to avoid unwanted Tx queue switching.
>>
>> Signed-off-by: Wally Zhao <wallyzhao@...il.com>
>> ---
>>  net/sctp/output.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index dbda7e7..5ff75cc 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -626,6 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet,
>> gfp_t gfp)
>>  	/* neighbour should be confirmed on successful transmission or
>>  	 * positive error
>>  	 */
>> +
>> +	/* allow switch tx queue only for the first in flight pkt */
>> +	head->ooo_okay = asoc->outqueue.outstanding_bytes == 0;
>
> Considering we are talking about NIC queues here, we would have a
> better result with tp->flight_size instead. As in, we can switch
> queues if, for this transport, the queue is empty.
>
>> +
>>  	if (tp->af_specific->sctp_xmit(head, tp) >= 0 &&
>>  	    tp->dst_pending_confirm)
>>  		tp->dst_pending_confirm = 0;
>> --
>> 1.8.3.1
>>
>