Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

[Matthieu Baerts <matttbe@kernel.org>]

Hi Mat,

On 16/02/2024 00:25, Mat Martineau wrote:
> On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:
> 
>> After the 'Fixes' commit mentioned below, the client side might print
>> the following warning once when a subflow is fully established at the
>> reception of any valid additional ack:
>>
>>  MPTCP: bogus mpc option on established client sk
>>
>> That's a normal situation, and no warning should be printed for that. We
>> can then skip the check when the label is used.
>>
>> Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields
>> initialization")
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Looks good to me:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

Thank you for the review!

>> ---
>> Notes:
>> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>>   our tree? Or just in DEBUG mode?
> 
> I think it makes sense to keep this patch minimal for -net and stable
> (just moving the label).
> 
> Also given the consequences of panic_on_warn, would be better to make
> any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
> complexity to modify this warning in our tree or debug mode as being
> worth it, do you think it would be valuable?

Sorry, I'm a bit confused by your reply. For the moment, our CI
complains when a "Call Trace:" is printed, but it doesn't complain when
there is a pr_warn().

If the warning here can be caused by interactions with a buggy host, it
makes sense not to have a WARN() when used in production, but it would
be good for our CI can catch that. If the warning can only be caused by
an internal bug, then easier to use a WARN_ONCE().

So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
and upstream that. WDYT?

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