Re: [PATCH mptcp-net v2] mptcp: pr_debug: add missing \n at the end

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 21/08/2024 08:46, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2024-08-19 at 10:01 +0200, Matthieu Baerts (NGI0) wrote:
>> pr_debug() have been added in various places in MPTCP code to help
>> developers to debug some situations. With the dynamic debug feature,
>> it
>> is easy to enable all or some of them, and asks users to reproduce
>> issues with extra debug.
>>
>> Many of these pr_debug() don't end with a new line, while no
>> 'pr_cont()'
>> are used in MPTCP code. So the goal was not to display multiple debug
>> messages on one line: they were then not missing the '\n' on purpose.
>> Not having the new line at the end causes these messages to be
>> printed
>> with a delay, when something else needs to be printed. This issue is
>> not
>> visible when many messages need to be printed, but it is annoying and
>> confusing when only specific messages are expected, e.g.
>>
>>   # echo "func mptcp_pm_add_addr_echoed +fmp" >
>> /sys/kernel/debug/dynamic_debug/control
>>   # ./mptcp_join.sh "signal address"; \
>>         echo "$(awk '{print $1}' /proc/uptime) - end"; \
>>         sleep 5s; \
>>         echo "$(awk '{print $1}' /proc/uptime) - restart"; \
>>         ./mptcp_join.sh "signal address"
>>   013 signal address
>>       (...)
>>   551.75 - end
>>   556.76 - restart
>>   013 signal address
>>   [  551.367935] mptcp:mptcp_pm_add_addr_echoed: MPTCP:
>> msk=000000000740d23c
>>       (...)
>>
>>   ## => a delay of 5 seconds.
>>
>> The 'Fixes' tag here below points to the first pr_debug() used
>> without
>> '\n' in net/mptcp. This patch could be split in many small ones, with
>> different Fixes tag, but it doesn't seem worth it, because it is easy
>> to
>> re-generate this patch with this simple 'sed' command:
>>
>>   git grep -l pr_debug -- net/mptcp |
>>     xargs sed -i "s/\(pr_debug(\".*[^n]\)\(\"[,)]\)/\1\\\n\2/g"
>>
>> So in case of conflicts, simply drop the modifications, and launch
>> this
>> command.
>>
>> Fixes: f870fa0b5768 ("mptcp: Add MPTCP socket stubs")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Thanks for this v2. LGTM!
> 
> Reviewed-by: Geliang Tang <geliang@kernel.org>

Thank you for the review!

New patches for t/upstream-net and t/upstream:
- adcccec8d800: mptcp: pr_debug: add missing \n at the end
- Results: 76f6fba9ef6b..ab5c53816256 (export-net)
- Results: 80d8c6b850fb..624d90dc5dca (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/d0a1f54fb2f9a9a0e2b6ea85fae9670ac4818469/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/f81bdeae843224cb42ef2613e833d46675d9e8c9/checks

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