[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> Feel free to tell me I have to do any other modifications (e.g. expose
> tcp_request_sock_ipv6_ops).

Are you done with your rebase work?

I'd like to propose another set of fixups to get rid
of refcounting and the kbuild robot warnings reported with rfcv2.

I don't want to start doing this while export is in flux.

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

Hi Florian,

On 08/10/2019 14:15, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> Feel free to tell me I have to do any other modifications (e.g. expose
>> tcp_request_sock_ipv6_ops).
> 
> Are you done with your rebase work?

Yes, I don't have anything planned for the moment, waiting for orders :-)

> I'd like to propose another set of fixups to get rid
> of refcounting and the kbuild robot warnings reported with rfcv2.

I can help if needed, if you prefer to create patch at the end of the 
series and I "squash" them in TopGit.

> I don't want to start doing this while export is in flux.

Thank you for the notification!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

Hi Mat,

On 08/10/2019 11:08, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 08/10/2019 00:54, Mat Martineau wrote:
>>
>> Matthieu,
>>
>> On Mon, 7 Oct 2019, Matthieu Baerts wrote:
>>
>>> On 07/10/2019 17:23, Florian Westphal wrote:
>>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>>> On 07/10/2019 17:00, Florian Westphal wrote:
>>>>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>>>>>> ... this turns 'tcp: clean ext on tx recycle' into a one-line 
>>>>>>>> change.
>>>>>>>
>>>>>>> Good idea! Just applied this diff and added your signed-off to 
>>>>>>> the patch.
>>>>>>
>>>>>> Thanks!
>>>>>
>>>>> Just in case you want to check, here is the diff between my two 
>>>>> branches:
>>>>
>>>> [..]
>>>>
>>>>> If there is no objection, I am going to re-create the TopGit tree 
>>>>> with this
>>>>> new branch then!
>>>>
>>>> Looks good, go ahead.
>>>
>>> TopGit tree re-created, export branch has been recreated, tests are 
>>> still OK.
>>>
>>
>>
>> I have one more change to suggest. In "tcp: Expose tcp struct and 
>> routine for MPTCP", we don't need to expose tcp_v4_init_sock() - that 
>> was probably associated with the pre-ULP code.
> 
> Good catch!
> 
>> That reduces the patch to only exporting tcp_request_sock_ipv4_ops, 
>> which I would suggest squashing with "tcp: Export low-level TCP 
>> functions"

I already started the modifications, for the moment, only the squash. I 
didn't move the commit nor exported tcp_request_sock_ipv6_ops.

- 73abc7855679: modify the commit message (I have to do that in two 
different commits for TG, not sure why)
- d20e51f3bbc3: partly squash 'tcp: Expose tcp struct and routine for 
MPTCP' into 'tcp: Export TCP functions and ops struct'
- 32fd0f33f52f: revert topic t/tcp-Expose-tcp-struct-and-routine-for-MPTCP
- 67ec4e697687: remove empty topic 
t/tcp-Expose-tcp-struct-and-routine-for-MPTCP
- 0e4e909653ea..846470882b58: result

Tests are still OK!

Feel free to tell me I have to do any other modifications (e.g. expose 
tcp_request_sock_ipv6_ops).

Cheers,
Matt

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

Hi Mat,

On 08/10/2019 00:54, Mat Martineau wrote:
> 
> Matthieu,
> 
> On Mon, 7 Oct 2019, Matthieu Baerts wrote:
> 
>> On 07/10/2019 17:23, Florian Westphal wrote:
>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>> On 07/10/2019 17:00, Florian Westphal wrote:
>>>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>>>>> ... this turns 'tcp: clean ext on tx recycle' into a one-line 
>>>>>>> change.
>>>>>>
>>>>>> Good idea! Just applied this diff and added your signed-off to the 
>>>>>> patch.
>>>>>
>>>>> Thanks!
>>>>
>>>> Just in case you want to check, here is the diff between my two 
>>>> branches:
>>>
>>> [..]
>>>
>>>> If there is no objection, I am going to re-create the TopGit tree 
>>>> with this
>>>> new branch then!
>>>
>>> Looks good, go ahead.
>>
>> TopGit tree re-created, export branch has been recreated, tests are 
>> still OK.
>>
> 
> 
> I have one more change to suggest. In "tcp: Expose tcp struct and 
> routine for MPTCP", we don't need to expose tcp_v4_init_sock() - that 
> was probably associated with the pre-ULP code.

Good catch!

> That reduces the patch to only exporting tcp_request_sock_ipv4_ops, 
> which I would suggest squashing with "tcp: Export low-level TCP 
> functions"

That's a good idea!

While I am on it, I think it would be better to move this to the end of 
the first part of the series as it is just exposing things needed for 
later, no bug fix if the code is extended in a specific way or 
introduction to MPTCP. But maybe that's a detail?


Also, regarding tcp_request_sock_ipv4_ops, should we already expose the 
IPv6 version?


====
diff --git a/include/net/tcp.h b/include/net/tcp.h
index daa35c2b9584..138aec1e2ed8 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1986,6 +1986,11 @@ struct tcp_request_sock_ops {
                            enum tcp_synack_type synack_type);
  };

+extern const struct tcp_request_sock_ops tcp_request_sock_ipv4_ops;
+#if IS_ENABLED(CONFIG_IPV6)
+extern const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops;
+#endif
+
  #ifdef CONFIG_SYN_COOKIES
  static inline __u32 cookie_init_sequence(const struct 
tcp_request_sock_ops *ops,
                                          const struct sock *sk, struct 
sk_buff *skb,
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5cb0e7f065ea..ea926e4c13f1 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1369,7 +1369,7 @@ struct request_sock_ops tcp_request_sock_ops 
__read_mostly = {
         .syn_ack_timeout =      tcp_syn_ack_timeout,
  };

-static const struct tcp_request_sock_ops tcp_request_sock_ipv4_ops = {
+const struct tcp_request_sock_ops tcp_request_sock_ipv4_ops = {
         .mss_clamp      =       TCP_MSS_DEFAULT,
  #ifdef CONFIG_TCP_MD5SIG
         .req_md5_lookup =       tcp_v4_md5_lookup,
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e3d9f4559c99..fbf34f575a29 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -784,7 +784,7 @@ struct request_sock_ops tcp6_request_sock_ops 
__read_mostly = {
         .syn_ack_timeout =      tcp_syn_ack_timeout,
  };

-static const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops = {
+const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops = {
         .mss_clamp      =       IPV6_MIN_MTU - sizeof(struct tcphdr) -
                                 sizeof(struct ipv6hdr),
  #ifdef CONFIG_TCP_MD5SIG
====


WDYT?

> and then updating the commit message:
> 
> """
> tcp: Export TCP functions and ops struct
> 
> MPTCP will make use of tcp_send_mss() and tcp_push() when sending
> data to specific TCP subflows.
> 
> tcp_request_sock_ipv4_ops will be referenced during TCP subflow creation.
> 
> """

Thank you for the message, I will update it too!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Mat Martineau]

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


Matthieu,

On Mon, 7 Oct 2019, Matthieu Baerts wrote:

> On 07/10/2019 17:23, Florian Westphal wrote:
>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>> On 07/10/2019 17:00, Florian Westphal wrote:
>>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>>>> ... this turns 'tcp: clean ext on tx recycle' into a one-line change.
>>>>> 
>>>>> Good idea! Just applied this diff and added your signed-off to the 
>>>>> patch.
>>>> 
>>>> Thanks!
>>> 
>>> Just in case you want to check, here is the diff between my two branches:
>> 
>> [..]
>> 
>>> If there is no objection, I am going to re-create the TopGit tree with 
>>> this
>>> new branch then!
>> 
>> Looks good, go ahead.
>
> TopGit tree re-created, export branch has been recreated, tests are still OK.
>


I have one more change to suggest. In "tcp: Expose tcp struct and routine 
for MPTCP", we don't need to expose tcp_v4_init_sock() - that was probably 
associated with the pre-ULP code.

That reduces the patch to only exporting tcp_request_sock_ipv4_ops, which 
I would suggest squashing with "tcp: Export low-level TCP functions" and 
then updating the commit message:

"""
tcp: Export TCP functions and ops struct

MPTCP will make use of tcp_send_mss() and tcp_push() when sending
data to specific TCP subflows.

tcp_request_sock_ipv4_ops will be referenced during TCP subflow creation.

"""



--
Mat Martineau
Intel

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

On 07/10/2019 17:23, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> On 07/10/2019 17:00, Florian Westphal wrote:
>>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>>>> ... this turns 'tcp: clean ext on tx recycle' into a one-line change.
>>>>
>>>> Good idea! Just applied this diff and added your signed-off to the patch.
>>>
>>> Thanks!
>>
>> Just in case you want to check, here is the diff between my two branches:
> 
> [..]
> 
>> If there is no objection, I am going to re-create the TopGit tree with this
>> new branch then!
> 
> Looks good, go ahead.

TopGit tree re-created, export branch has been recreated, tests are 
still OK.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> On 07/10/2019 17:00, Florian Westphal wrote:
> > Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > > > ... this turns 'tcp: clean ext on tx recycle' into a one-line change.
> > > 
> > > Good idea! Just applied this diff and added your signed-off to the patch.
> > 
> > Thanks!
> 
> Just in case you want to check, here is the diff between my two branches:

[..]

> If there is no objection, I am going to re-create the TopGit tree with this
> new branch then!

Looks good, go ahead.

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

On 07/10/2019 17:00, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>> ... this turns 'tcp: clean ext on tx recycle' into a one-line change.
>>
>> Good idea! Just applied this diff and added your signed-off to the patch.
> 
> Thanks!

Just in case you want to check, here is the diff between my two branches:

$ git diff rebase-net-tcp-first..rebase-net-tcp-first-v2
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c6748a4eb51b..71ffca02cb61 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4102,14 +4102,6 @@ static inline void skb_ext_put(struct sk_buff *skb)
                 __skb_ext_put(skb->extensions);
  }

-static inline void skb_ext_clear(struct sk_buff *skb)
-{
-       if (skb->active_extensions) {
-               __skb_ext_put(skb->extensions);
-               skb->active_extensions = 0;
-       }
-}
-
  static inline void __skb_ext_copy(struct sk_buff *dst,
                                   const struct sk_buff *src)
  {
diff --git a/include/net/sock.h b/include/net/sock.h
index 97ad8c62af1d..84ea0efe7952 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1470,7 +1470,7 @@ static inline void sk_wmem_free_skb(struct sock 
*sk, struct sk_buff *skb)
         sk_mem_uncharge(sk, skb->truesize);
         if (static_branch_unlikely(&tcp_tx_skb_cache_key) &&
             !sk->sk_tx_skb_cache && !skb_cloned(skb)) {
-               skb_ext_clear(skb);
+               skb_ext_reset(skb);
                 skb_zcopy_clear(skb, true);
                 sk->sk_tx_skb_cache = skb;
                 return;


If there is no objection, I am going to re-create the TopGit tree with 
this new branch then!

>>> If we place this early in the series, then
>>>
>>>> 966eb30045b9 mptcp: increment MIB counters in a few places
>>>
>>> ... could be folded into the patches that add those code paths.
>>>
>>> Perhaps right after 'mptcp: Add MPTCP socket stubs'?
>>>
>>> Could also be squashed, I do not mind.  Just a suggestion.
>>
>> For me it is clearer to have dedicated patches for the introduction of the
>> MIBs: it might help reviewers to easily point out "strategic places" or
>> because spending less time on that because it only modifies MPTCP code :)
> 
> Right, we can do this as a followup so it doesn't influence the initial
> batch sizes in any way.
> 
> In light of this I agree its better to keep it as-is.
> 
>> It is just that I guess the reviewers will not like having too big patches
>> neither. I understand that 40 patches is too big but as a reviewer, I would
>> prefer having a very few more patches and split per features / refactoring.
> 
> Same here.
> 
>> I mean: if we send the same modifications in less patches, I don't know if
>> it will help reviewers.
> 
> Right.  Lets keep it separate.

Great!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > ... this turns 'tcp: clean ext on tx recycle' into a one-line change.
> 
> Good idea! Just applied this diff and added your signed-off to the patch.

Thanks!

> > If we place this early in the series, then
> > 
> > > 966eb30045b9 mptcp: increment MIB counters in a few places
> > 
> > ... could be folded into the patches that add those code paths.
> > 
> > Perhaps right after 'mptcp: Add MPTCP socket stubs'?
> > 
> > Could also be squashed, I do not mind.  Just a suggestion.
> 
> For me it is clearer to have dedicated patches for the introduction of the
> MIBs: it might help reviewers to easily point out "strategic places" or
> because spending less time on that because it only modifies MPTCP code :)

Right, we can do this as a followup so it doesn't influence the initial
batch sizes in any way.

In light of this I agree its better to keep it as-is.

> It is just that I guess the reviewers will not like having too big patches
> neither. I understand that 40 patches is too big but as a reviewer, I would
> prefer having a very few more patches and split per features / refactoring.

Same here.

> I mean: if we send the same modifications in less patches, I don't know if
> it will help reviewers.

Right.  Lets keep it separate.

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Matthieu Baerts]

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

Hi Florian,

Thank you for the quick review!

On 07/10/2019 16:11, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> Hi,
>>
>> I just did a rebase to have these commits at the top:
>>
>>    net: Make sock protocol value checks more specific
>>    sock: Make sk_protocol a 16-bit value
>>    tcp: Define IPPROTO_MPTCP
>>    # new # tcp: Add MPTCP option number
>>    tcp, ulp: Add clone operation to tcp_ulp_ops
>>    # new # mptcp: Add MPTCP to skb extensions
>>    tcp: Prevent coalesce/collapse when skb has MPTCP extensions (requires
>> MPTCP skb extensions)
>>    tcp: Export low-level TCP functions
>>    tcp: Check for filled TCP option space before SACK
>>    tcp: clean ext on tx recycle
>>    tcp: Expose tcp struct and routine for MPTCP
>>
>> The work is visible in my repo, branch "rebase-net-tcp-first"
> 
> Thanks.  A few comments related to squashing and code-churn reduction
> below:
> 
>> Here is the new order:
>>
>> ac0ee9246e87 net: Make sock protocol value checks more specific
>> 62ea284edd7e sock: Make sk_protocol a 16-bit value
>> 62f49d1713cd tcp: Define IPPROTO_MPTCP
>> 4897684a3794 tcp: Add MPTCP option number
>> 497c810b48ad tcp, ulp: Add clone operation to tcp_ulp_ops
>> 8d38f78f68e0 mptcp: Add MPTCP to skb extensions
>> e71e2810b8de tcp: Prevent coalesce/collapse when skb has MPTCP extensions
>> a341f52a58d9 tcp: Export low-level TCP functions
>> f77bcc800d76 tcp: Check for filled TCP option space before SACK
> 
> LGTM, thanks!
> 
>> 20fe333f058b tcp: clean ext on tx recycle
> 
> Can you squash this small patch into this one?
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4099,14 +4099,6 @@ static inline void skb_ext_put(struct sk_buff *skb)
>   		__skb_ext_put(skb->extensions);
>   }
>   
> -static inline void skb_ext_clear(struct sk_buff *skb)
> -{
> -	if (skb->active_extensions) {
> -		__skb_ext_put(skb->extensions);
> -		skb->active_extensions = 0;
> -	}
> -}
> -
>   static inline void __skb_ext_copy(struct sk_buff *dst,
>   				  const struct sk_buff *src)
>   {
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 97ad8c62af1d..84ea0efe7952 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1470,7 +1470,7 @@ static inline void sk_wmem_free_skb(struct sock *sk, struct sk_buff *skb)
>   	sk_mem_uncharge(sk, skb->truesize);
>   	if (static_branch_unlikely(&tcp_tx_skb_cache_key) &&
>   	    !sk->sk_tx_skb_cache && !skb_cloned(skb)) {
> -		skb_ext_clear(skb);
> +		skb_ext_reset(skb);
>   		skb_zcopy_clear(skb, true);
>   		sk->sk_tx_skb_cache = skb;
>   		return;
> 
> ... this turns 'tcp: clean ext on tx recycle' into a one-line change.

Good idea! Just applied this diff and added your signed-off to the patch.

>> dde2a56add38 mptcp: Write MPTCP DSS headers to outgoing data packets
> 
> This patch adds
> 
> #define MPTCP_DSS_DATA_FIN     BIT(4)
> 
> and others to include/net/mptcp.h
> 
>> 50da2ff90d38 mptcp: Implement MPTCP receive path
> 
> ... then this patch moves them to net/mptcp/protocol.h.
> 
> it would be better to already place them in protocol.h in the
> previous patch.

Good catch! Just moved them to net/mptcp/protocol.h

Here is the new refs (pushed in rebase-net-tcp-first-v2 branch) just in 
case someone wants to comment with the new commits.

ac0ee9246e87 net: Make sock protocol value checks more specific
62ea284edd7e sock: Make sk_protocol a 16-bit value
62f49d1713cd tcp: Define IPPROTO_MPTCP
4897684a3794 tcp: Add MPTCP option number
497c810b48ad tcp, ulp: Add clone operation to tcp_ulp_ops
8d38f78f68e0 mptcp: Add MPTCP to skb extensions
e71e2810b8de tcp: Prevent coalesce/collapse when skb has MPTCP extensions
a341f52a58d9 tcp: Export low-level TCP functions
f77bcc800d76 tcp: Check for filled TCP option space before SACK
fb53381dd218 tcp: clean ext on tx recycle
81fd95a12fd0 tcp: Expose tcp struct and routine for MPTCP
56a8a7d8cf27 mptcp: Add MPTCP socket stubs
85b4bf829962 mptcp: Handle MPTCP TCP options
3d9f941e1975 mptcp: Associate MPTCP context with TCP socket
699c0cd5beaa mptcp: Handle MP_CAPABLE options for outgoing connections
ff728beb1bb3 mptcp: add mptcp_poll
f54ef1eb9739 mptcp: Create SUBFLOW socket for incoming connections
7a2be03cc042 mptcp: Add key generation and token tree
56358e799f22 mptcp: Add shutdown() socket operation
46c5a6978ae9 mptcp: Add setsockopt()/getsockopt() socket operations
c1856853e204 mptcp: Write MPTCP DSS headers to outgoing data packets
8ff4c1c4c391 mptcp: Implement MPTCP receive path
7f71a708c8e0 mptcp: use sk_page_frag() in sendmsg
ef698e744e61 mptcp: sendmsg() do spool all the provided data
4d32f2cd8a35 mptcp: allow collapsing consecutive sendpages on the same 
substream
fe20b87caca3 mptcp: Add path manager interface
1b33b4ec3277 mptcp: Add ADD_ADDR handling
0cf4a2738861 mptcp: Add handling of incoming MP_JOIN requests
d4a2a2111fe9 mptcp: harmonize locking on all socket operations.
fe4b85f96742 mptcp: new sysctl to control the activation per NS
133ad374c072 mptcp: add basic kselftest for mptcp
7ad456eb359d mptcp: Add handling of outgoing MP_JOIN requests
64335e3bdcbd mptcp: Implement path manager interface commands
170077363215 mptcp: Make MPTCP socket block/wakeup ignore sk_receive_queue
c08ddcfa7b50 mptcp: update per unacked sequence on pkt reception
a0dcd3524ad2 mptcp: queue data for mptcp level retransmission
4d5ed3b0e7ad mptcp: introduce MPTCP retransmission timer
1032786bd03e mptcp: implement memory accounting for mptcp rtx queue
179d8e49880d mptcp: rework mptcp_sendmsg_frag to accept optional dfrag
ca1f24e19a1e mptcp: implement and use MPTCP-level retransmission
ac8935fe70e4 mptcp: allow dumping subflow context to userspace
d88ddbcddcd5 mptcp: add MIB counter infrastructure
c5716a92dc7e mptcp: increment MIB counters in a few places

>> 8f991f422f4c mptcp: add MIB counter infrastructure
> 
> If we place this early in the series, then
> 
>> 966eb30045b9 mptcp: increment MIB counters in a few places
> 
> ... could be folded into the patches that add those code paths.
> 
> Perhaps right after 'mptcp: Add MPTCP socket stubs'?
> 
> Could also be squashed, I do not mind.  Just a suggestion.

For me it is clearer to have dedicated patches for the introduction of 
the MIBs: it might help reviewers to easily point out "strategic places" 
or because spending less time on that because it only modifies MPTCP code :)
But if we have to limit the number of patches, we can of course do that.

It is just that I guess the reviewers will not like having too big 
patches neither. I understand that 40 patches is too big but as a 
reviewer, I would prefer having a very few more patches and split per 
features / refactoring. I mean: if we send the same modifications in 
less patches, I don't know if it will help reviewers. But of course, 
better squashing patches that fix bugs introduced in other patches of 
the series.
So for me, I understood that it is better not to squash too much but 
send our modifications by smaller sets.

But I understand your point and incrementing MIBs counters could be done 
directly when we introduce the action.

Just let me know if you think it will be required by upstream to squash 
them and I can work on that. :-)

(but this is less urgent if we only want to send the 11th patches we 
listed last week, right?)

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [GIT] move TCP-related commits to the beginning

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> Hi,
> 
> I just did a rebase to have these commits at the top:
> 
>   net: Make sock protocol value checks more specific
>   sock: Make sk_protocol a 16-bit value
>   tcp: Define IPPROTO_MPTCP
>   # new # tcp: Add MPTCP option number
>   tcp, ulp: Add clone operation to tcp_ulp_ops
>   # new # mptcp: Add MPTCP to skb extensions
>   tcp: Prevent coalesce/collapse when skb has MPTCP extensions (requires
> MPTCP skb extensions)
>   tcp: Export low-level TCP functions
>   tcp: Check for filled TCP option space before SACK
>   tcp: clean ext on tx recycle
>   tcp: Expose tcp struct and routine for MPTCP
> 
> The work is visible in my repo, branch "rebase-net-tcp-first"

Thanks.  A few comments related to squashing and code-churn reduction
below:

> Here is the new order:
> 
> ac0ee9246e87 net: Make sock protocol value checks more specific
> 62ea284edd7e sock: Make sk_protocol a 16-bit value
> 62f49d1713cd tcp: Define IPPROTO_MPTCP
> 4897684a3794 tcp: Add MPTCP option number
> 497c810b48ad tcp, ulp: Add clone operation to tcp_ulp_ops
> 8d38f78f68e0 mptcp: Add MPTCP to skb extensions
> e71e2810b8de tcp: Prevent coalesce/collapse when skb has MPTCP extensions
> a341f52a58d9 tcp: Export low-level TCP functions
> f77bcc800d76 tcp: Check for filled TCP option space before SACK

LGTM, thanks!

> 20fe333f058b tcp: clean ext on tx recycle

Can you squash this small patch into this one?
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4099,14 +4099,6 @@ static inline void skb_ext_put(struct sk_buff *skb)
 		__skb_ext_put(skb->extensions);
 }
 
-static inline void skb_ext_clear(struct sk_buff *skb)
-{
-	if (skb->active_extensions) {
-		__skb_ext_put(skb->extensions);
-		skb->active_extensions = 0;
-	}
-}
-
 static inline void __skb_ext_copy(struct sk_buff *dst,
 				  const struct sk_buff *src)
 {
diff --git a/include/net/sock.h b/include/net/sock.h
index 97ad8c62af1d..84ea0efe7952 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1470,7 +1470,7 @@ static inline void sk_wmem_free_skb(struct sock *sk, struct sk_buff *skb)
 	sk_mem_uncharge(sk, skb->truesize);
 	if (static_branch_unlikely(&tcp_tx_skb_cache_key) &&
 	    !sk->sk_tx_skb_cache && !skb_cloned(skb)) {
-		skb_ext_clear(skb);
+		skb_ext_reset(skb);
 		skb_zcopy_clear(skb, true);
 		sk->sk_tx_skb_cache = skb;
 		return;

... this turns 'tcp: clean ext on tx recycle' into a one-line change.

> dde2a56add38 mptcp: Write MPTCP DSS headers to outgoing data packets

This patch adds

#define MPTCP_DSS_DATA_FIN     BIT(4)

and others to include/net/mptcp.h

> 50da2ff90d38 mptcp: Implement MPTCP receive path

... then this patch moves them to net/mptcp/protocol.h.

it would be better to already place them in protocol.h in the
previous patch.

> 8f991f422f4c mptcp: add MIB counter infrastructure

If we place this early in the series, then

> 966eb30045b9 mptcp: increment MIB counters in a few places

... could be folded into the patches that add those code paths.

Perhaps right after 'mptcp: Add MPTCP socket stubs'?

Could also be squashed, I do not mind.  Just a suggestion.