Re: [PATCH mptcp-next 2/3] selftests: mptcp: join: tolerate more ADD_ADDR

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/09/2025 11:26, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2025-09-02 at 21:01 +0200, Matthieu Baerts (NGI0) wrote:
>> ADD_ADDR can be retransmitted, and with, the parent commit, these
>> retransmissions can be sent quicker: from 2 minutes to less than one
>> second.
>>
>> To avoid false positives where retransmitted ADD_ADDR causes higher
>> counters than expected, it is required to be more tolerant. Errors
>> are
>> now only reported when fewer ADD_ADDRs have been sent/received.
>>
>> An alternative could be to disable the ADD_ADDR retransmissions by
>> default, but that's changing the default kernel behaviour. Plus,
>> ADD_ADDR retransmissions can be required for some tests. To avoid
>> adding
>> exceptions to a few tests, it seems better to increase the tolerance.
> 
> I think adding "tolerate more ADD_ADDR" to the two general helpers
> chk_add_nr() and chk_add_tx_nr() is not very accurate, because we don't
> know which test sends/receives more ADD_ADDR. Because chk_add_nr() and
> chk_add_tx_nr() of most tests are normal, only a few tests will be
> affected and need to add "tolerate".
> 
> I think it can be handled like chk_csum_nr(), accepting a number with
> "+", such as "+2" to allow more than two ADD_ADDR. In this way, we only
> need to add this "+" to a few failed tests, so the control is more
> accurate. What do you think?

I initially did something similar: marking the tests where we can be
more tolerant, but the modifications were quite important. But that was
not it: before, there were no retransmissions (only after 2 minutes, so
after the end of each subtest), and we had the tolerance only when
setting the timeout to 1 second. Now, we always have retransmissions
(and we don't control the timeout any more), so to be aligned with what
we had before, we should probably always have this tolerance. We might
have unexpected retransmissions because suddenly, the host got busy, and
the ADD_ADDR retransmission timer fires. We don't want to be impacted by
these unexpected events. I think that's valid for all tests were we
check the ADD_ADDR.

Maybe what is missing is an exception for the expected counter 0: if we
expect no ADD_ADDR, we should fail if some are seen. But this issue was
already there, we would need another patch for that, probably. (I can look).

Also, what would be certainly better here, is to add a new MIB counters
for the retransmissions, so we can check the actual number instead of
having the tolerance. Again, that's orthogonal to this fix I think, so
this can be done later on.

So in short:
- this patch simply align with what was already there
- we should add an exception: no tolerance when expecting no ADD_ADDR
- we might add a new MIB counter (sent retransmitted ADD_ADDR) to avoid
the tolerance (on newer kernels only: when this counter is available)

WDYT?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.