[MPTCP] Re: [PATCH mptcp-next] docs: networking: mptcp: Add MPTCP sysctl entries

[MPTCP] Re: [PATCH mptcp-next] docs: networking: mptcp: Add MPTCP sysctl entries

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 2735 bytes --]

Hi Mat,

Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年11月3日周二 上午9:55写道：
>
> Describe the two MPTCP sysctls, what the values mean, and the default
> settings.
>
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
>
> Jakub requested sysctl documentation for the 'add_addr_timeout' setting,
> and we didn't have our 'enabled' sysctl documented either.
>
> If this looks good to everyone I can add it to v2 the miscellaneous
> series on the netdev list.
>

Thanks for your help.

Acked-by: Geliang Tang <geliangtang(a)gmail.com>

-Geliang

>
>  Documentation/networking/index.rst        |  1 +
>  Documentation/networking/mptcp-sysctl.rst | 24 +++++++++++++++++++++++
>  MAINTAINERS                               |  1 +
>  3 files changed, 26 insertions(+)
>  create mode 100644 Documentation/networking/mptcp-sysctl.rst
>
> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> index 63ef386afd0a..70c71c9206e2 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -70,6 +70,7 @@ Contents:
>     lapb-module
>     mac80211-injection
>     mpls-sysctl
> +   mptcp-sysctl
>     multiqueue
>     netconsole
>     netdev-features
> diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
> new file mode 100644
> index 000000000000..70c076eb361d
> --- /dev/null
> +++ b/Documentation/networking/mptcp-sysctl.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +====================
> +MPTCP Sysfs variables
> +====================
> +
> +/proc/sys/net/mptcp/* Variables:
> +===============================
> +
> +enabled - INTEGER
> +       Control whether MPTCP sockets can be created.
> +
> +       MPTCP sockets can be created if the value is nonzero.
> +
> +       Default: 1
> +
> +add_addr_timeout - INTEGER (seconds)
> +       Set the timeout after which an ADD_ADDR control message will be
> +       resent to an MPTCP peer that has not acknowledged a previous
> +       ADD_ADDR message.
> +
> +       The default value matches TCP_RTO_MAX.
> +
> +       Default: 120
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 17f5571788c9..badaaa815aa3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12265,6 +12265,7 @@ L:      mptcp(a)lists.01.org
>  S:     Maintained
>  W:     https://github.com/multipath-tcp/mptcp_net-next/wiki
>  B:     https://github.com/multipath-tcp/mptcp_net-next/issues
> +F:     Documentation/networking/mptcp-sysctl.rst
>  F:     include/net/mptcp.h
>  F:     include/uapi/linux/mptcp.h
>  F:     net/mptcp/
> --
> 2.29.2
>

[MPTCP] Re: [PATCH mptcp-next] docs: networking: mptcp: Add MPTCP sysctl entries

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

Hi Mat,

On 03/11/2020 02:55, Mat Martineau wrote:
> Describe the two MPTCP sysctls, what the values mean, and the default
> settings.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
> ---
> 
> Jakub requested sysctl documentation for the 'add_addr_timeout' setting,
> and we didn't have our 'enabled' sysctl documented either.

Thanks for looking after this!

> If this looks good to everyone I can add it to v2 the miscellaneous
> series on the netdev list.

Good idea! I just have one remark below!

(...)

> diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst
> new file mode 100644
> index 000000000000..70c076eb361d
> --- /dev/null
> +++ b/Documentation/networking/mptcp-sysctl.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +====================
> +MPTCP Sysfs variables
> +====================

The "s" at the end is going to fall :-/

More seriously, if I am not mistaken, it is important to have at least 
the same amount of characters for the mark representing the title in RST.

There are some RST validators or maybe try this: http://rst.ninjs.org

> +/proc/sys/net/mptcp/* Variables:
> +===============================

Same here: one "=" is missing.

Note that for me, it is strange to add a ":" at the end of a title. They 
don't do that in ip-sysctl.rst but they do that in mpls-sysctl.rst. 
Personally, I would not add it.

Should we give a very short introduction about MPTCP here? I guess not 
in this "-sysctl" file. Later when we will have a documentation for 
userspace.

> +enabled - INTEGER
> +	Control whether MPTCP sockets can be created.

Should we mention this?

   This is a per-namespace sysctl.

For me, that's a very useful thing to know but often missing in the doc.

> +	MPTCP sockets can be created if the value is nonzero.
> +
> +	Default: 1
> +
> +add_addr_timeout - INTEGER (seconds)
> +	Set the timeout after which an ADD_ADDR control message will be
> +	resent to an MPTCP peer that has not acknowledged a previous
> +	ADD_ADDR message.

Here too, it is per-ns. I guess it would be good to repeat that for each 
sysctl. But because all of them are per-ns, we can also add something in 
the introduction, just under "/proc/sys/net/mptcp/* Variables". Up to you!

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

[MPTCP] Re: [PATCH mptcp-next] docs: networking: mptcp: Add MPTCP sysctl entries

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]


On Tue, 3 Nov 2020, Matthieu Baerts wrote:

> Hi Mat,
>
> On 03/11/2020 02:55, Mat Martineau wrote:
>> Describe the two MPTCP sysctls, what the values mean, and the default
>> settings.
>> 
>> Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>
>> ---
>> 
>> Jakub requested sysctl documentation for the 'add_addr_timeout' setting,
>> and we didn't have our 'enabled' sysctl documented either.
>
> Thanks for looking after this!
>
>> If this looks good to everyone I can add it to v2 the miscellaneous
>> series on the netdev list.
>
> Good idea! I just have one remark below!
>
> (...)
>
>> diff --git a/Documentation/networking/mptcp-sysctl.rst 
>> b/Documentation/networking/mptcp-sysctl.rst
>> new file mode 100644
>> index 000000000000..70c076eb361d
>> --- /dev/null
>> +++ b/Documentation/networking/mptcp-sysctl.rst
>> @@ -0,0 +1,24 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +====================
>> +MPTCP Sysfs variables
>> +====================
>
> The "s" at the end is going to fall :-/
>
> More seriously, if I am not mistaken, it is important to have at least the 
> same amount of characters for the mark representing the title in RST.
>
> There are some RST validators or maybe try this: http://rst.ninjs.org

Ok, will fix that up.

>
>> +/proc/sys/net/mptcp/* Variables:
>> +===============================
>
> Same here: one "=" is missing.
>
> Note that for me, it is strange to add a ":" at the end of a title. They 
> don't do that in ip-sysctl.rst but they do that in mpls-sysctl.rst. 
> Personally, I would not add it.
>
> Should we give a very short introduction about MPTCP here? I guess not in 
> this "-sysctl" file. Later when we will have a documentation for userspace.

Yes, I did start from a copy of mpls-sysctl.rst and inherited its quirks.

Happy to remove the colon. We do need to add MPTCP documentation but for 
now I think we can start with the sysctl section.

>
>> +enabled - INTEGER
>> +	Control whether MPTCP sockets can be created.
>
> Should we mention this?
>
>  This is a per-namespace sysctl.
>
> For me, that's a very useful thing to know but often missing in the doc.
>
>> +	MPTCP sockets can be created if the value is nonzero.
>> +
>> +	Default: 1
>> +
>> +add_addr_timeout - INTEGER (seconds)
>> +	Set the timeout after which an ADD_ADDR control message will be
>> +	resent to an MPTCP peer that has not acknowledged a previous
>> +	ADD_ADDR message.
>
> Here too, it is per-ns. I guess it would be good to repeat that for each 
> sysctl. But because all of them are per-ns, we can also add something in the 
> introduction, just under "/proc/sys/net/mptcp/* Variables". Up to you!
>

Will add the per-ns information.

Thanks for the review!

--
Mat Martineau
Intel