[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Peter Krystad]

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

On Tue, 2020-01-21 at 15:04 +0100, Matthieu Baerts wrote:
> Hi Peter,

<snip>

> Or do you think it will be mandatory to have this optimization now?

Not mandatory, we can go as is. I realized (after some code review) just this
morning that for v1 although we do not send/receive MPC+DSS we need to have 
space for both options at the same time. This fragment when processing
MP_CAPABLE in mptcp_parse_option() shows the need:

	if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA) {
		/* Section 3.1.:
		 * "the data parameters in a MP_CAPABLE are semantically
		 * equivalent to those in a DSS option and can be used
		 * interchangeably."
		 */
		mp_opt->dss = 1;

The code for part 2 is fine as is. I'll send the optimizations for ADD_ADDR,
etc. later.

Peter.

> Cheers,
> Matt

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Matthieu Baerts]

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

Hi Peter,

On 21/01/2020 18:13, Peter Krystad wrote:
> On Tue, 2020-01-21 at 15:04 +0100, Matthieu Baerts wrote:
>> Hi Peter,
> 
> <snip>
> 
>> Or do you think it will be mandatory to have this optimization now?
> 
> Not mandatory, we can go as is. I realized (after some code review) just this
> morning that for v1 although we do not send/receive MPC+DSS we need to have
> space for both options at the same time. This fragment when processing
> MP_CAPABLE in mptcp_parse_option() shows the need:
> 
> 	if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA) {
> 		/* Section 3.1.:
> 		 * "the data parameters in a MP_CAPABLE are semantically
> 		 * equivalent to those in a DSS option and can be used
> 		 * interchangeably."
> 		 */
> 		mp_opt->dss = 1;

Good catch, we have to deal with v0 and v1. Then indeed better not to 
modify part 2 and do that in part 3.

> The code for part 2 is fine as is. I'll send the optimizations for ADD_ADDR,
> etc. later.

Thank you!

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: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Matthieu Baerts]

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

Hi Peter,

On 21/01/2020 06:15, Peter Krystad wrote:
> On Mon, 2020-01-20 at 23:34 +0100, Matthieu Baerts wrote:
>> Hi Paolo, Peter, Mat,
>>
>> On 19/01/2020 15:27, Paolo Abeni wrote:
>>> On Fri, 2020-01-17 at 14:00 -0800, Peter Krystad wrote:
>>>> On Fri, 2020-01-17 at 19:40 +0100, Paolo Abeni wrote:
>>>>> On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
>>>>>> On Thu, 16 Jan 2020, Peter Krystad wrote:
>>>>>>
>>>>>>> Use union to minimize space used in structure.
>>>>>>> Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
>>>>>>>
>>>>>>> squashto: Implement MPTCP receive path
>>>>>>>
>>>>>>> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
>>>>>>> ---
>>>>>>> include/linux/tcp.h | 18 ++++++++++++------
>>>>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>>>>>>> index 6abf8d9f1a42..a9200a86a6d2 100644
>>>>>>> --- a/include/linux/tcp.h
>>>>>>> +++ b/include/linux/tcp.h
>>>>>>> @@ -80,12 +80,18 @@ struct tcp_sack_block {
>>>>>>>
>>>>>>> #if IS_ENABLED(CONFIG_MPTCP)
>>>>>>> struct mptcp_options_received {
>>>>>>> -	u64	sndr_key;
>>>>>>> -	u64	rcvr_key;
>>>>>>> -	u32	data_ack32;
>>>>>>> -	u32	data_seq32;
>>>>>>> -	u64	data_ack;
>>>>>>> -	u64	data_seq;
>>>>>>> +	union {
>>>>>>> +		struct {
>>>>>>> +			u64	sndr_key;
>>>>>>> +			u64	rcvr_key;
>>>>>>> +			u32	data_ack32;
>>>>>>> +			u32	data_seq32;
>>>>>>> +		};
>>>>>>> +		struct {
>>>>>>> +			u64	data_ack;
>>>>>>> +			u64	data_seq;
>>>>>>> +		};
>>>>>>> +	};
>>>>>>
>>>>>> If one of the goals is to ensure that mixed 32/64 ack/seq is working (as
>>>>>> mentioned in next commit message), I think it would be better to express
>>>>>> that as a union with the data_ack/data_seq and sndr_key/rcvr_key values
>>>>>> rather than depend on struct alignment:
>>>>>>
>>>>>>    	struct {
>>>>>>    		union {
>>>>>>    			struct {
>>>>>>    				u64	sndr_key;
>>>>>>    				u64	rcvr_key;
>>>>>>    			};
>>>>>>    			struct {
>>>>>>    				u64	data_ack;
>>>>>>    				u64	data_seq;
>>>>>>    			};
>>>>>>    		};
>>>>>>    		u32	data_ack32;
>>>>>>    		u32	data_seq32;
>>>>>>    	};
>>>>>
>>>>> I'm really low on coffee here, so please perdom me for the likely dumb
>>>>> comment, but...
>>>>>
>>>>> why do we need data_*32 and  *_key simult? I think we don't allow MPC
>>>>> and DSS in the same packet ?!?
>>>>
>>>> The mutipath-tcp.org implementation would do this so (and also data_*32 + IPv4
>>>> add_addr). If we know we won't ever do these combinations, and we only want to
>>>> work with ourselves, then it is not necessary.
>>>
>>> Uhmm.. I *think* with MPTCP v1 MPC+DSS is no longer required (nor
>>> should be sent)? we have MPC+data instead...
>>>
>>> (sorry I have the vague the feeling we already have discussed this in
>>> the past, but for sure I completely forgot the conclusion...)
>>
>> Sorry to ask this question but do we want this change for part 2?
>>
>> I understand the benefits and we should optimize mptcp_options_received
>> structure at some points but from what I see, there are still some
>> discussions about the content of the structure and having this union
>> forces us to make sure we don't mix the usage of the members when
>> parsing TCP options and when reading what has been parsed. Maybe it's a
>> risk we don't want to take now for part 2.
>>
>> We could postpone these optimizations for later before adding MP_JOIN
>> and ADD_ADDR.
>>
>> This patch and the "parent" one ("mptcp: Store 32 and 64-bit ACKs
>> separately") are the two blocking ones to finalize part 2.
>>
>> What do you think?
> 
> I still think the optimization is worth addressing.

Me too.

> Here is what I propose:
> 
> I will drop/withdraw the two commits regarding 32-bit ack/seq:
> mptcp: Store 32 and 64-bit ACKs separately
> mptcp: Re-factor expand_ack
> 
> Then I will re-submit the 'optimize' patches revised to reflect the
> dropping of the first two, adding checks to make sure use of the union
> is safe and to disallow simultaneous MPC+DSS. If we want to defer these
> until after v2 is submitted (to reduce risk and time to submission) that
> is also fine with me.

I think it would be better to defer these optimizations.

For part 2 where we don't support MP_JOIN, ADD_ADDR, RM_ADDR, MP_FAIL, 
MP_PRIO and MP_FASTCLOSE, we can wait before adding complexity here. So 
in this patch-set with support of MP_CAPABLE (/!\ v0 and later v1) and 
DSS, I guess we can only save max 64bits per TCP socket which is nothing 
when you look at its current size.

If the maintainers wants us to already do this optimization job now, we 
can integrate what you are working on later. But at least we can have a 
review on the rest of the code as well :-)

Or do you think it will be mandatory to have this optimization now?

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: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Peter Krystad]

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

On Mon, 2020-01-20 at 23:34 +0100, Matthieu Baerts wrote:
> Hi Paolo, Peter, Mat,
> 
> On 19/01/2020 15:27, Paolo Abeni wrote:
> > On Fri, 2020-01-17 at 14:00 -0800, Peter Krystad wrote:
> > > On Fri, 2020-01-17 at 19:40 +0100, Paolo Abeni wrote:
> > > > On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
> > > > > On Thu, 16 Jan 2020, Peter Krystad wrote:
> > > > > 
> > > > > > Use union to minimize space used in structure.
> > > > > > Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
> > > > > > 
> > > > > > squashto: Implement MPTCP receive path
> > > > > > 
> > > > > > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > > > > > ---
> > > > > > include/linux/tcp.h | 18 ++++++++++++------
> > > > > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > > > > > index 6abf8d9f1a42..a9200a86a6d2 100644
> > > > > > --- a/include/linux/tcp.h
> > > > > > +++ b/include/linux/tcp.h
> > > > > > @@ -80,12 +80,18 @@ struct tcp_sack_block {
> > > > > > 
> > > > > > #if IS_ENABLED(CONFIG_MPTCP)
> > > > > > struct mptcp_options_received {
> > > > > > -	u64	sndr_key;
> > > > > > -	u64	rcvr_key;
> > > > > > -	u32	data_ack32;
> > > > > > -	u32	data_seq32;
> > > > > > -	u64	data_ack;
> > > > > > -	u64	data_seq;
> > > > > > +	union {
> > > > > > +		struct {
> > > > > > +			u64	sndr_key;
> > > > > > +			u64	rcvr_key;
> > > > > > +			u32	data_ack32;
> > > > > > +			u32	data_seq32;
> > > > > > +		};
> > > > > > +		struct {
> > > > > > +			u64	data_ack;
> > > > > > +			u64	data_seq;
> > > > > > +		};
> > > > > > +	};
> > > > > 
> > > > > If one of the goals is to ensure that mixed 32/64 ack/seq is working (as
> > > > > mentioned in next commit message), I think it would be better to express
> > > > > that as a union with the data_ack/data_seq and sndr_key/rcvr_key values
> > > > > rather than depend on struct alignment:
> > > > > 
> > > > >   	struct {
> > > > >   		union {
> > > > >   			struct {
> > > > >   				u64	sndr_key;
> > > > >   				u64	rcvr_key;
> > > > >   			};
> > > > >   			struct {
> > > > >   				u64	data_ack;
> > > > >   				u64	data_seq;
> > > > >   			};
> > > > >   		};
> > > > >   		u32	data_ack32;
> > > > >   		u32	data_seq32;
> > > > >   	};
> > > > 
> > > > I'm really low on coffee here, so please perdom me for the likely dumb
> > > > comment, but...
> > > > 
> > > > why do we need data_*32 and  *_key simult? I think we don't allow MPC
> > > > and DSS in the same packet ?!?
> > > 
> > > The mutipath-tcp.org implementation would do this so (and also data_*32 + IPv4
> > > add_addr). If we know we won't ever do these combinations, and we only want to
> > > work with ourselves, then it is not necessary.
> > 
> > Uhmm.. I *think* with MPTCP v1 MPC+DSS is no longer required (nor
> > should be sent)? we have MPC+data instead...
> > 
> > (sorry I have the vague the feeling we already have discussed this in
> > the past, but for sure I completely forgot the conclusion...)
> 
> Sorry to ask this question but do we want this change for part 2?
> 
> I understand the benefits and we should optimize mptcp_options_received 
> structure at some points but from what I see, there are still some 
> discussions about the content of the structure and having this union 
> forces us to make sure we don't mix the usage of the members when 
> parsing TCP options and when reading what has been parsed. Maybe it's a 
> risk we don't want to take now for part 2.
> 
> We could postpone these optimizations for later before adding MP_JOIN 
> and ADD_ADDR.
> 
> This patch and the "parent" one ("mptcp: Store 32 and 64-bit ACKs 
> separately") are the two blocking ones to finalize part 2.
> 
> What do you think?

I still think the optimization is worth addressing. Here is what I propose:

I will drop/withdraw the two commits regarding 32-bit ack/seq:
mptcp: Store 32 and 64-bit ACKs separately
mptcp: Re-factor expand_ack

Then I will re-submit the 'optimize' patches revised to reflect the
dropping of the first two, adding checks to make sure use of the union
is safe and to disallow simultaneous MPC+DSS. If we want to defer these
until after v2 is submitted (to reduce risk and time to submission) that
is also fine with me.

Peter.
 
> Cheers,
> Matt

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Matthieu Baerts]

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

Hi Paolo, Peter, Mat,

On 19/01/2020 15:27, Paolo Abeni wrote:
> On Fri, 2020-01-17 at 14:00 -0800, Peter Krystad wrote:
>> On Fri, 2020-01-17 at 19:40 +0100, Paolo Abeni wrote:
>>> On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
>>>> On Thu, 16 Jan 2020, Peter Krystad wrote:
>>>>
>>>>> Use union to minimize space used in structure.
>>>>> Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
>>>>>
>>>>> squashto: Implement MPTCP receive path
>>>>>
>>>>> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
>>>>> ---
>>>>> include/linux/tcp.h | 18 ++++++++++++------
>>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>>>>> index 6abf8d9f1a42..a9200a86a6d2 100644
>>>>> --- a/include/linux/tcp.h
>>>>> +++ b/include/linux/tcp.h
>>>>> @@ -80,12 +80,18 @@ struct tcp_sack_block {
>>>>>
>>>>> #if IS_ENABLED(CONFIG_MPTCP)
>>>>> struct mptcp_options_received {
>>>>> -	u64	sndr_key;
>>>>> -	u64	rcvr_key;
>>>>> -	u32	data_ack32;
>>>>> -	u32	data_seq32;
>>>>> -	u64	data_ack;
>>>>> -	u64	data_seq;
>>>>> +	union {
>>>>> +		struct {
>>>>> +			u64	sndr_key;
>>>>> +			u64	rcvr_key;
>>>>> +			u32	data_ack32;
>>>>> +			u32	data_seq32;
>>>>> +		};
>>>>> +		struct {
>>>>> +			u64	data_ack;
>>>>> +			u64	data_seq;
>>>>> +		};
>>>>> +	};
>>>>
>>>> If one of the goals is to ensure that mixed 32/64 ack/seq is working (as
>>>> mentioned in next commit message), I think it would be better to express
>>>> that as a union with the data_ack/data_seq and sndr_key/rcvr_key values
>>>> rather than depend on struct alignment:
>>>>
>>>>   	struct {
>>>>   		union {
>>>>   			struct {
>>>>   				u64	sndr_key;
>>>>   				u64	rcvr_key;
>>>>   			};
>>>>   			struct {
>>>>   				u64	data_ack;
>>>>   				u64	data_seq;
>>>>   			};
>>>>   		};
>>>>   		u32	data_ack32;
>>>>   		u32	data_seq32;
>>>>   	};
>>>
>>> I'm really low on coffee here, so please perdom me for the likely dumb
>>> comment, but...
>>>
>>> why do we need data_*32 and  *_key simult? I think we don't allow MPC
>>> and DSS in the same packet ?!?
>>
>> The mutipath-tcp.org implementation would do this so (and also data_*32 + IPv4
>> add_addr). If we know we won't ever do these combinations, and we only want to
>> work with ourselves, then it is not necessary.
> 
> 
> Uhmm.. I *think* with MPTCP v1 MPC+DSS is no longer required (nor
> should be sent)? we have MPC+data instead...
> 
> (sorry I have the vague the feeling we already have discussed this in
> the past, but for sure I completely forgot the conclusion...)

Sorry to ask this question but do we want this change for part 2?

I understand the benefits and we should optimize mptcp_options_received 
structure at some points but from what I see, there are still some 
discussions about the content of the structure and having this union 
forces us to make sure we don't mix the usage of the members when 
parsing TCP options and when reading what has been parsed. Maybe it's a 
risk we don't want to take now for part 2.

We could postpone these optimizations for later before adding MP_JOIN 
and ADD_ADDR.

This patch and the "parent" one ("mptcp: Store 32 and 64-bit ACKs 
separately") are the two blocking ones to finalize part 2.

What do you think?

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: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Paolo Abeni]

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

On Fri, 2020-01-17 at 14:00 -0800, Peter Krystad wrote:
> On Fri, 2020-01-17 at 19:40 +0100, Paolo Abeni wrote:
> > On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
> > > On Thu, 16 Jan 2020, Peter Krystad wrote:
> > > 
> > > > Use union to minimize space used in structure.
> > > > Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
> > > > 
> > > > squashto: Implement MPTCP receive path
> > > > 
> > > > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > > > ---
> > > > include/linux/tcp.h | 18 ++++++++++++------
> > > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > > > index 6abf8d9f1a42..a9200a86a6d2 100644
> > > > --- a/include/linux/tcp.h
> > > > +++ b/include/linux/tcp.h
> > > > @@ -80,12 +80,18 @@ struct tcp_sack_block {
> > > > 
> > > > #if IS_ENABLED(CONFIG_MPTCP)
> > > > struct mptcp_options_received {
> > > > -	u64	sndr_key;
> > > > -	u64	rcvr_key;
> > > > -	u32	data_ack32;
> > > > -	u32	data_seq32;
> > > > -	u64	data_ack;
> > > > -	u64	data_seq;
> > > > +	union {
> > > > +		struct {
> > > > +			u64	sndr_key;
> > > > +			u64	rcvr_key;
> > > > +			u32	data_ack32;
> > > > +			u32	data_seq32;
> > > > +		};
> > > > +		struct {
> > > > +			u64	data_ack;
> > > > +			u64	data_seq;
> > > > +		};
> > > > +	};
> > > 
> > > If one of the goals is to ensure that mixed 32/64 ack/seq is working (as 
> > > mentioned in next commit message), I think it would be better to express 
> > > that as a union with the data_ack/data_seq and sndr_key/rcvr_key values 
> > > rather than depend on struct alignment:
> > > 
> > >  	struct {
> > >  		union {
> > >  			struct {
> > >  				u64	sndr_key;
> > >  				u64	rcvr_key;
> > >  			};
> > >  			struct {
> > >  				u64	data_ack;
> > >  				u64	data_seq;
> > >  			};
> > >  		};
> > >  		u32	data_ack32;
> > >  		u32	data_seq32;
> > >  	};
> > 
> > I'm really low on coffee here, so please perdom me for the likely dumb
> > comment, but... 
> > 
> > why do we need data_*32 and  *_key simult? I think we don't allow MPC
> > and DSS in the same packet ?!?
> 
> The mutipath-tcp.org implementation would do this so (and also data_*32 + IPv4
> add_addr). If we know we won't ever do these combinations, and we only want to
> work with ourselves, then it is not necessary.


Uhmm.. I *think* with MPTCP v1 MPC+DSS is no longer required (nor
should be sent)? we have MPC+data instead...

(sorry I have the vague the feeling we already have discussed this in
the past, but for sure I completely forgot the conclusion...)

Thanks,

Paolo

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Peter Krystad]

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

On Fri, 2020-01-17 at 19:40 +0100, Paolo Abeni wrote:
> On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
> > On Thu, 16 Jan 2020, Peter Krystad wrote:
> > 
> > > Use union to minimize space used in structure.
> > > Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
> > > 
> > > squashto: Implement MPTCP receive path
> > > 
> > > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > > ---
> > > include/linux/tcp.h | 18 ++++++++++++------
> > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > > index 6abf8d9f1a42..a9200a86a6d2 100644
> > > --- a/include/linux/tcp.h
> > > +++ b/include/linux/tcp.h
> > > @@ -80,12 +80,18 @@ struct tcp_sack_block {
> > > 
> > > #if IS_ENABLED(CONFIG_MPTCP)
> > > struct mptcp_options_received {
> > > -	u64	sndr_key;
> > > -	u64	rcvr_key;
> > > -	u32	data_ack32;
> > > -	u32	data_seq32;
> > > -	u64	data_ack;
> > > -	u64	data_seq;
> > > +	union {
> > > +		struct {
> > > +			u64	sndr_key;
> > > +			u64	rcvr_key;
> > > +			u32	data_ack32;
> > > +			u32	data_seq32;
> > > +		};
> > > +		struct {
> > > +			u64	data_ack;
> > > +			u64	data_seq;
> > > +		};
> > > +	};
> > 
> > If one of the goals is to ensure that mixed 32/64 ack/seq is working (as 
> > mentioned in next commit message), I think it would be better to express 
> > that as a union with the data_ack/data_seq and sndr_key/rcvr_key values 
> > rather than depend on struct alignment:
> > 
> >  	struct {
> >  		union {
> >  			struct {
> >  				u64	sndr_key;
> >  				u64	rcvr_key;
> >  			};
> >  			struct {
> >  				u64	data_ack;
> >  				u64	data_seq;
> >  			};
> >  		};
> >  		u32	data_ack32;
> >  		u32	data_seq32;
> >  	};
> 
> I'm really low on coffee here, so please perdom me for the likely dumb
> comment, but... 
> 
> why do we need data_*32 and  *_key simult? I think we don't allow MPC
> and DSS in the same packet ?!?

The mutipath-tcp.org implementation would do this so (and also data_*32 + IPv4
add_addr). If we know we won't ever do these combinations, and we only want to
work with ourselves, then it is not necessary.

Peter.

> Thanks,
> 
> Paolo
> 

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Paolo Abeni]

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

On Fri, 2020-01-17 at 10:08 -0800, Mat Martineau wrote:
> On Thu, 16 Jan 2020, Peter Krystad wrote:
> 
> > Use union to minimize space used in structure.
> > Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
> > 
> > squashto: Implement MPTCP receive path
> > 
> > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > ---
> > include/linux/tcp.h | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index 6abf8d9f1a42..a9200a86a6d2 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -80,12 +80,18 @@ struct tcp_sack_block {
> > 
> > #if IS_ENABLED(CONFIG_MPTCP)
> > struct mptcp_options_received {
> > -	u64	sndr_key;
> > -	u64	rcvr_key;
> > -	u32	data_ack32;
> > -	u32	data_seq32;
> > -	u64	data_ack;
> > -	u64	data_seq;
> > +	union {
> > +		struct {
> > +			u64	sndr_key;
> > +			u64	rcvr_key;
> > +			u32	data_ack32;
> > +			u32	data_seq32;
> > +		};
> > +		struct {
> > +			u64	data_ack;
> > +			u64	data_seq;
> > +		};
> > +	};
> 
> If one of the goals is to ensure that mixed 32/64 ack/seq is working (as 
> mentioned in next commit message), I think it would be better to express 
> that as a union with the data_ack/data_seq and sndr_key/rcvr_key values 
> rather than depend on struct alignment:
> 
>  	struct {
>  		union {
>  			struct {
>  				u64	sndr_key;
>  				u64	rcvr_key;
>  			};
>  			struct {
>  				u64	data_ack;
>  				u64	data_seq;
>  			};
>  		};
>  		u32	data_ack32;
>  		u32	data_seq32;
>  	};

I'm really low on coffee here, so please perdom me for the likely dumb
comment, but... 

why do we need data_*32 and  *_key simult? I think we don't allow MPC
and DSS in the same packet ?!?

Thanks,

Paolo

[MPTCP] Re: [PATCH] mptcp: Optimize DSS fields in mptcp_options_received

[Mat Martineau]

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

On Thu, 16 Jan 2020, Peter Krystad wrote:

> Use union to minimize space used in structure.
> Allow 32-bit DSS to be received at the same time as MP_CAPABLE.
>
> squashto: Implement MPTCP receive path
>
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
> include/linux/tcp.h | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 6abf8d9f1a42..a9200a86a6d2 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -80,12 +80,18 @@ struct tcp_sack_block {
>
> #if IS_ENABLED(CONFIG_MPTCP)
> struct mptcp_options_received {
> -	u64	sndr_key;
> -	u64	rcvr_key;
> -	u32	data_ack32;
> -	u32	data_seq32;
> -	u64	data_ack;
> -	u64	data_seq;
> +	union {
> +		struct {
> +			u64	sndr_key;
> +			u64	rcvr_key;
> +			u32	data_ack32;
> +			u32	data_seq32;
> +		};
> +		struct {
> +			u64	data_ack;
> +			u64	data_seq;
> +		};
> +	};

If one of the goals is to ensure that mixed 32/64 ack/seq is working (as 
mentioned in next commit message), I think it would be better to express 
that as a union with the data_ack/data_seq and sndr_key/rcvr_key values 
rather than depend on struct alignment:

 	struct {
 		union {
 			struct {
 				u64	sndr_key;
 				u64	rcvr_key;
 			};
 			struct {
 				u64	data_ack;
 				u64	data_seq;
 			};
 		};
 		u32	data_ack32;
 		u32	data_seq32;
 	};

That does require some rearranging in later commits (reintroducing the 
top-level union to add the ipv6 ADD_ADDR data).

> 	u32	subflow_seq;
> 	u16	data_len;
> 	u8	mp_capable : 1,
> -- 
> 2.17.2

--
Mat Martineau
Intel