[PATCH mptcp-next] net: skip printing "link become ready" v6 msg

[PATCH mptcp-next] net: skip printing "link become ready" v6 msg

[Matthieu Baerts]

This following message is printed in the console each time a network
device configured with an IPv6 addresses is ready to be used:

  ADDRCONF(NETDEV_CHANGE): <iface>: link becomes ready

When netns are being extensively used, e.g. by re-creating netns with
veth to discuss with each other for testing purposes like mptcp_join.sh
selftest is doing, it generates a lot of messages: more than 700 with
the latest version.

This message can be useful in many situations but in some, it floods the
logs without providing any useful input. The proposition here is to have
a new sysctl knob to easily skip this specific message.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 include/net/netns/ipv6.h                        | 1 +
 net/ipv6/addrconf.c                             | 5 +++--
 net/ipv6/sysctl_net_ipv6.c                      | 9 +++++++++
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index 3cceb3e9320b..721abf86052f 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -56,6 +56,7 @@ struct netns_sysctl_ipv6 {
 	bool skip_notify_on_dev_down;
 	u8 fib_notify_on_flag_change;
 	u8 icmpv6_error_anycast_as_unicast;
+	bool skip_print_link_becomes_ready;
 };
 
 struct netns_ipv6 {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3797917237d0..9cf7b4932309 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3633,8 +3633,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 				idev->if_flags |= IF_READY;
 			}
 
-			pr_info("ADDRCONF(NETDEV_CHANGE): %s: link becomes ready\n",
-				dev->name);
+			if (!net->ipv6.sysctl.skip_print_link_becomes_ready)
+				pr_info("ADDRCONF(NETDEV_CHANGE): %s: link becomes ready\n",
+					dev->name);
 
 			run_pending = 1;
 		}
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index 94a0a294c6a1..c9e82377a8fa 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -213,6 +213,15 @@ static struct ctl_table ipv6_table_template[] = {
 		.proc_handler	= proc_doulongvec_minmax,
 		.extra2		= &ioam6_id_wide_max,
 	},
+	{
+		.procname	= "skip_print_link_becomes_ready",
+		.data		= &init_net.ipv6.sysctl.skip_print_link_becomes_ready,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1         = SYSCTL_ZERO,
+		.extra2         = SYSCTL_ONE,
+	},
 	{ }
 };
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 0044d87556dd..27179e9175be 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -84,6 +84,7 @@ init_partial()
 		ip netns exec $netns sysctl -q net.mptcp.pm_type=0
 		ip netns exec $netns sysctl -q net.ipv4.conf.all.rp_filter=0
 		ip netns exec $netns sysctl -q net.ipv4.conf.default.rp_filter=0
+		ip netns exec $netns sysctl -q net.ipv6.skip_print_link_becomes_ready=1 2>/dev/null || true
 		if [ $checksum -eq 1 ]; then
 			ip netns exec $netns sysctl -q net.mptcp.checksum_enabled=1
 		fi

---
base-commit: 194dd0efe579cb5d3a746d248b3476f4b3fc0b48
change-id: 20230522-mptcp-skip_print_link_becomes_ready-af50c71a9daa

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

Re: net: skip printing "link become ready" v6 msg: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5210575714648064
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5210575714648064/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6336475621490688
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6336475621490688/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6055000644780032
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6055000644780032/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 2 failed test(s): packetdrill_syscalls selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/4929100737937408
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4929100737937408/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c8485501ba56


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next] net: skip printing "link become ready" v6 msg

[Mat Martineau]

On Mon, 22 May 2023, Matthieu Baerts wrote:

> This following message is printed in the console each time a network
> device configured with an IPv6 addresses is ready to be used:
>
>  ADDRCONF(NETDEV_CHANGE): <iface>: link becomes ready
>
> When netns are being extensively used, e.g. by re-creating netns with
> veth to discuss with each other for testing purposes like mptcp_join.sh
> selftest is doing, it generates a lot of messages: more than 700 with
> the latest version.
>
> This message can be useful in many situations but in some, it floods the
> logs without providing any useful input. The proposition here is to have
> a new sysctl knob to easily skip this specific message.
>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> include/net/netns/ipv6.h                        | 1 +
> net/ipv6/addrconf.c                             | 5 +++--
> net/ipv6/sysctl_net_ipv6.c                      | 9 +++++++++
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
> 4 files changed, 14 insertions(+), 2 deletions(-)
>

Hi Matthieu -

Using a sysctl for this seems like overkill. I noticed a similar change 
for ADDRCONF(NETDEV_UP) that changed a pr_info() to pr_debug():

7c62b8dd5ca89dabd9f455d19e663bad60951bd5

Maybe that's more suitable?


If you did want to keep pursuing the sysctl, the patch would need to 
update Documentation/networking/ip-sysctl.rst


Thanks,
Mat


> diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
> index 3cceb3e9320b..721abf86052f 100644
> --- a/include/net/netns/ipv6.h
> +++ b/include/net/netns/ipv6.h
> @@ -56,6 +56,7 @@ struct netns_sysctl_ipv6 {
> 	bool skip_notify_on_dev_down;
> 	u8 fib_notify_on_flag_change;
> 	u8 icmpv6_error_anycast_as_unicast;
> +	bool skip_print_link_becomes_ready;
> };
>
> struct netns_ipv6 {
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 3797917237d0..9cf7b4932309 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3633,8 +3633,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> 				idev->if_flags |= IF_READY;
> 			}
>
> -			pr_info("ADDRCONF(NETDEV_CHANGE): %s: link becomes ready\n",
> -				dev->name);
> +			if (!net->ipv6.sysctl.skip_print_link_becomes_ready)
> +				pr_info("ADDRCONF(NETDEV_CHANGE): %s: link becomes ready\n",
> +					dev->name);
>
> 			run_pending = 1;
> 		}
> diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
> index 94a0a294c6a1..c9e82377a8fa 100644
> --- a/net/ipv6/sysctl_net_ipv6.c
> +++ b/net/ipv6/sysctl_net_ipv6.c
> @@ -213,6 +213,15 @@ static struct ctl_table ipv6_table_template[] = {
> 		.proc_handler	= proc_doulongvec_minmax,
> 		.extra2		= &ioam6_id_wide_max,
> 	},
> +	{
> +		.procname	= "skip_print_link_becomes_ready",
> +		.data		= &init_net.ipv6.sysctl.skip_print_link_becomes_ready,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1         = SYSCTL_ZERO,
> +		.extra2         = SYSCTL_ONE,
> +	},
> 	{ }
> };
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 0044d87556dd..27179e9175be 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -84,6 +84,7 @@ init_partial()
> 		ip netns exec $netns sysctl -q net.mptcp.pm_type=0
> 		ip netns exec $netns sysctl -q net.ipv4.conf.all.rp_filter=0
> 		ip netns exec $netns sysctl -q net.ipv4.conf.default.rp_filter=0
> +		ip netns exec $netns sysctl -q net.ipv6.skip_print_link_becomes_ready=1 2>/dev/null || true
> 		if [ $checksum -eq 1 ]; then
> 			ip netns exec $netns sysctl -q net.mptcp.checksum_enabled=1
> 		fi
>
> ---
> base-commit: 194dd0efe579cb5d3a746d248b3476f4b3fc0b48
> change-id: 20230522-mptcp-skip_print_link_becomes_ready-af50c71a9daa
>
> Best regards,
> -- 
> Matthieu Baerts <matthieu.baerts@tessares.net>
>
>
>

Re: [PATCH mptcp-next] net: skip printing "link become ready" v6 msg

[Matthieu Baerts]

Hi Mat, Paolo,

@Mat: Thank you for having reviewed this patch!

@Paolo: I have a question for you below if you don't mind.

On 24/05/2023 02:20, Mat Martineau wrote:
> On Mon, 22 May 2023, Matthieu Baerts wrote:
> 
>> This following message is printed in the console each time a network
>> device configured with an IPv6 addresses is ready to be used:
>>
>>  ADDRCONF(NETDEV_CHANGE): <iface>: link becomes ready
>>
>> When netns are being extensively used, e.g. by re-creating netns with
>> veth to discuss with each other for testing purposes like mptcp_join.sh
>> selftest is doing, it generates a lot of messages: more than 700 with
>> the latest version.
>>
>> This message can be useful in many situations but in some, it floods the
>> logs without providing any useful input. The proposition here is to have
>> a new sysctl knob to easily skip this specific message.
>>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>> include/net/netns/ipv6.h                        | 1 +
>> net/ipv6/addrconf.c                             | 5 +++--
>> net/ipv6/sysctl_net_ipv6.c                      | 9 +++++++++
>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
>> 4 files changed, 14 insertions(+), 2 deletions(-)
>>
> 
> Hi Matthieu -
> 
> Using a sysctl for this seems like overkill. I noticed a similar change
> for ADDRCONF(NETDEV_UP) that changed a pr_info() to pr_debug():
> 
> 7c62b8dd5ca89dabd9f455d19e663bad60951bd5
> 
> Maybe that's more suitable?

I don't know. I mean: I don't even know if this message is that useful.
I quickly search for this message on the Internet and I found bug
reports because this message was abnormally printed. So in these cases,
it was a sign something else was wrong. Maybe the case in most use
cases? Even if there are other (better) ways to track issues with
network devices...

But yes, pr_debug() would be easier :)

The commit you mentioned also talk about the message I want to mute:

  (...) it seems to be more interesting to hear when the addrconf
  actually start (...)

(but it doesn't say if it is that useful)


@Paolo: should I ask on netdev which direction to take? Or send this
patch as a RFC (and mention the doc is missing and the modification on
the MPTCP side will be done in a separated commit + the pr_debug()
possibility)?

> If you did want to keep pursuing the sysctl, the patch would need to
> update Documentation/networking/ip-sysctl.rst

Good point, I forgot that!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net