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

[Jeffrey E Altman]

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: smime.p7s
Description: S/MIME Cryptographic Signature