Re: [PATCH 01/41] rxrpc: Fix RTT determination to use PING ACKs as a source

[Jeffrey E Altman <jaltman@auristor.com>]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]

On 11/9/2023 5:06 PM, David Howells wrote:
> I do not believe the ack_reason matters within rxrpc_input_ack(). As 
> long as
>> the acked_serial is non-zero,
>> rxrpc_complete_rtt_probe() can be called to attempt to compute an RTT.   If
>> there is an exact match for the
>> acked_serial then an RTT can be computed and if acked_serial is later than the
>> pending rtt probe, the probe
>> can be abandoned with the following caveats.
>>
>> 1. Receiving an acked_serial that is later than the serial of the
>>     transmitted probe indicates that a packet
>>     transmitted after the probe was received first.  Or that reordering
>>     of the transmitted packets occurred.
>>     Or that the probe was never received by the peer; or that the peer's
>>     response to the probe was lost in
>>     transit.
>> 2. The serial number namespace is unsigned 32-bit shared across all of
>>     the call channels of the associated
>>     rx connection.  As the serial numbers will wrap the use of after()
>>     within rxrpc_complete_rtt_probe to
>>     compare their values is questionable.   If serial numbers will be
>>     compared in this manner then they
>>     need to be locally tracked and compared as unsigned 64-bit values
>>     where only the low 32-bits are
>>     transmitted on the wire and any wire serial number equal to zero is
>>     ignored.
> I do ignore ack.serial == 0 for this purpose.

Zero has the special meaning - this ACK is not explicitly in response to 
a received packet.

However, as mentioned, the serial number counter wraps frequently and 
most RxRPC implementations
do not transition from serial 0xffffffff -> 0x00000001 when wrapping.

> I'm not sure how expanding it internally to 64-bits actually helps since the
> upper 32 bits is not visible to the peer.
rxrpc_complete_rtt_probe contains the following logic that relies on 
after() being able to detect
a serial number wrap.

         /* If a later serial is being acked, then mark this slot as
          * being available.
          */

         if (after(acked_serial, orig_serial)) {
             trace_rxrpc_rtt_rx(call, rxrpc_rtt_rx_obsolete, i,
                        orig_serial, acked_serial, 0, 0);
             clear_bit(i + RXRPC_CALL_RTT_PEND_SHIFT, &call->rtt_avail);
             smp_wmb();
             set_bit(i, &call->rtt_avail);
         }

Otherwise, acked_serial = 0x01 will be considered smaller than 
orig_serial = 0xfffffffe and the slot will not be marked available.

I will note that there is a similar problem with rxrpc_seq_t values 
which are u32 on the wire but which will wrap for calls that transmit 
more than approximately 5.5TB of data. Calls of this size are unlikely 
for a cache manager but are common for any service transmitting volume 
dumps.

Jeffrey Altman


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]