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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/09/2025 12:47, Matthieu Baerts wrote:
> 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).

To be more correct here: the issue was already there before, but we
couldn't hit it because "net.mptcp.add_addr_timeout" was only set to 1
(check with tolerance) when we expected to see ADD_ADDR. In other words,
no need to have an exception for 0 before this patch. I will then modify
this one.
> 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.

(Note that this might be nice, but I'm currently not planning to add this)

> 
> 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

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