7:56 a.m.
On 09/11/17 - 07:31:40, Ilya Lesokhin wrote:
One of the issues I see with TLS is that we need to update
This control buffer when SKBs are split or merged.
Have you given any thought into how it should be done?
In any case, having a hint with a pointer to record could help TLS
Even if the hint is not always accurate.
This would still incur the cost of maintaining and allocating the records in
the list. Do you have an estimate as to the cost of it in terms of CPU
cycles? (should be possible to have a rough measurement with perf)
In general, there seems to be a need for adding meta-data to skb's. (I just
looked at skb_shared_info->meta_len which was also added recently for XDP).
With all these use-cases, it might be worth to have something clean to store
this meta-data.
Another idea we had was to store meta-data rather somewhere in the linear
memory of the skb (e.g., between skb->head and skb->data or between
skb->end and skb->tail).
Christoph
> -----Original Message-----
> From: Boris Pismenny
> Sent: Thursday, November 09, 2017 8:48 AM
> To: cpaasch(a)apple.com
> Cc: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Ilya Lesokhin
> <ilyal(a)mellanox.com>; Liran Liss <liranl(a)mellanox.com>;
mptcp(a)lists.01.org
> Subject: RE: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
>
>
>
> > -----Original Message-----
> > From: cpaasch(a)apple.com [mailto:cpaasch@apple.com]
> > Sent: Thursday, November 09, 2017 13:48
> > To: Boris Pismenny <borisp(a)mellanox.com>
> > Cc: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Ilya Lesokhin
> > <ilyal(a)mellanox.com>; Liran Liss <liranl(a)mellanox.com>;
> > mptcp(a)lists.01.org
> > Subject: Re: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
> >
> > On 09/11/17 - 04:32:54, Boris Pismenny wrote:
> > > +Ilya and Liran
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: cpaasch(a)apple.com [mailto:cpaasch@apple.com]
> > > > Sent: Thursday, November 09, 2017 13:13
> > > > To: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Boris
> > Pismenny
> > > > <borisp(a)mellanox.com>
> > > > Cc: mptcp(a)lists.01.org
> > > > Subject: Re: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
> > > >
> > > > +Boris
> > > >
> > > > On 20/10/17 - 16:02:31, Mat Martineau wrote:
> > > > > The sk_buff control buffer is of limited size, and cannot be
> > > > > enlarged without significant impact on systemwide memory use.
> > > > > However,
> > additional
> > > > > per-packet state is needed for some protocols, like Multipath
TCP.
> > > > >
> > > > > An optional shared control buffer placed after the normal
struct
> > > > > skb_shared_info can accomodate the necessary state without
> > > > > imposing extra memory usage or code changes on normal struct
> > > > > sk_buff users. __alloc_skb will now place a skb_shared_info_ext
> > > > > structure at
> > > > > skb->end when given the SKB_ALLOC_SHINFO_EXT flag. Most users
of
> > struct
> > > > > sk_buff continue to use the skb_shinfo() macro to access shared
> > > > > info. skb_shinfo(skb)->is_ext is set if the extended
structure
> > > > > is available, and cleared if it is not.
> > > > >
> > > > > pskb_expand_head will preserve the shared control buffer if it
is
> present.
> > > > >
> > > > > Signed-off-by: Mat Martineau
> > > > > <mathew.j.martineau(a)linux.intel.com>
> > > > > ---
> > > > > include/linux/skbuff.h | 24 +++++++++++++++++++++-
> > > > > net/core/skbuff.c | 56
> ++++++++++++++++++++++++++++++++++++++--
> > -----
> > > > -----
> > > > > 2 files changed, 66 insertions(+), 14 deletions(-)
> > > >
> > > > Boris, below is the change I mentioned to you.
> > > >
> > > > It allows to allocate 48 additional bytes on-demand after
skb_shared_info.
> > > > As it is on-demand, it won't increase the size of the skb for
other users.
> > > >
> > > > For example, TLS could start using it when it creates the skb that
> > > > it pushes down to the TCP-stack. That way you don't need to
handle
> > > > the tls_record lists.
> > > >
> > >
> > > One small problem is that TLS doesn't create SKBs. As a ULP it calls
> > > the
> > transport send
> > > Functions (do_tcp_sendpages for TLS). This function receives a page
> > > and not a
> > SKB.
> >
> > yes, that's a good point. Mat has another patch as part of this
> > series, that adds an skb-arg to sendpages
> >
> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> > 01.org%2Fpipermail%2Fmptcp%2F2017-
> >
> October%2F000130.html&data=02%7C01%7Cborisp%40mellanox.com%7C8d
> 0
> >
> 4f7b1c39649a6e80d08d5272d17fe%7Ca652971c7d2e4d9ba6a4d149256f461
> b
> >
> %7C0%7C0%7C636457996996299024&sdata=YjycapXvL2N%2FkrPj15oTy2igh
> C
> > 23j1lKmWetdJtdXLU%3D&reserved=0)
> >
> > That should do the job for you.
> >
> > > We decided not to create the SKB outside of the TCP layer to reduce
> > > the
> > number of
> > > changes we made to TCP.
> > >
> > > It would be nice if we could use something like that. Did you talk
> > > to DaveM
> > about
> > > upstreaming this?
> >
> > No, we didn't talk yet to anyone outside of this list here about it.
> >
> > We were looking for a user of it outside of MPTCP but couldn't find one.
> > Now, it seems like we found one that would be interested :)
> >
> >
> > I think this infrastructure here would simplify your code quite a bit, no?
>
> I'm not sure. I'll need to think about that.
>
> I'm worried that in some cases it won't work, i.e. the shared_info
wouldn't
> reach our driver and then again we would be forced to use the tls_get_record
> function.
> One example, is the tcp_mtu_probe function, which creates a new skb. Another
> is tcp_collapse_retrans. We need to ensure all of these are covered when we
> pass any offloaded TLS skb to the driver.
>
> >
> > > We will definitely find it useful for the receive side, there we
> > > allocate the SKB
> > in the driver.
> >
> > Interesting! So even there you could use it. We were under the
> > impression that it would be of less interest for the receive-side.
> >
> >
> > Christoph
> >
> > >
> > > > See below for the rest of the patch.
> > > >
> > > >
> > > > Christoph
> > > >
12:31 a.m.
New subject: [PATCH 1/2] skbuff: Add shared control buffer
On Thu, 9 Nov 2017, cpaasch(a)apple.com wrote:
On 09/11/17 - 07:31:40, Ilya Lesokhin wrote:
> One of the issues I see with TLS is that we need to update
> This control buffer when SKBs are split or merged.
> Have you given any thought into how it should be done?
I'm trying to not update this control buffer after the skb is created for
MPTCP.
In the MPTCP case, when packets are split the newly created skb doesn't
need the metadata and can be a regular skb. The original skb (with data
truncated) keeps the metadata.
For MPTCP merging, I'm going to try using TCP_SKB_CB(skb)->eor to prevent
the collapse of an extended MPTCP skb in to a previous skb by the generic
TCP code. MPTCP could combine skbs in the write queue if needed since it
understands the extended information.
TLS probably has different constraints, but I thought I'd offer some more
context.
> In any case, having a hint with a pointer to record could help
TLS
> Even if the hint is not always accurate.
This would still incur the cost of maintaining and allocating the records in
the list. Do you have an estimate as to the cost of it in terms of CPU
cycles? (should be possible to have a rough measurement with perf)
In general, there seems to be a need for adding meta-data to skb's. (I just
looked at skb_shared_info->meta_len which was also added recently for XDP).
With all these use-cases, it might be worth to have something clean to store
this meta-data.
I agree. skb_shared_info->meta_len seems limited (using the mac header
pointer to determine the location of metadata). A cleaner way to store
metadata might let MPTCP and TLS be used at the same time.
Mat
Another idea we had was to store meta-data rather somewhere in the linear
memory of the skb (e.g., between skb->head and skb->data or between
skb->end and skb->tail).
Christoph
>
>> -----Original Message-----
>> From: Boris Pismenny
>> Sent: Thursday, November 09, 2017 8:48 AM
>> To: cpaasch(a)apple.com
>> Cc: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Ilya Lesokhin
>> <ilyal(a)mellanox.com>; Liran Liss <liranl(a)mellanox.com>;
mptcp(a)lists.01.org
>> Subject: RE: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
>>
>>
>>
>>> -----Original Message-----
>>> From: cpaasch(a)apple.com [mailto:cpaasch@apple.com]
>>> Sent: Thursday, November 09, 2017 13:48
>>> To: Boris Pismenny <borisp(a)mellanox.com>
>>> Cc: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Ilya Lesokhin
>>> <ilyal(a)mellanox.com>; Liran Liss <liranl(a)mellanox.com>;
>>> mptcp(a)lists.01.org
>>> Subject: Re: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
>>>
>>> On 09/11/17 - 04:32:54, Boris Pismenny wrote:
>>>> +Ilya and Liran
>>>>
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: cpaasch(a)apple.com [mailto:cpaasch@apple.com]
>>>>> Sent: Thursday, November 09, 2017 13:13
>>>>> To: Mat Martineau <mathew.j.martineau(a)linux.intel.com>; Boris
>>> Pismenny
>>>>> <borisp(a)mellanox.com>
>>>>> Cc: mptcp(a)lists.01.org
>>>>> Subject: Re: [MPTCP] [PATCH 1/2] skbuff: Add shared control buffer
>>>>>
>>>>> +Boris
>>>>>
>>>>> On 20/10/17 - 16:02:31, Mat Martineau wrote:
>>>>>> The sk_buff control buffer is of limited size, and cannot be
>>>>>> enlarged without significant impact on systemwide memory use.
>>>>>> However,
>>> additional
>>>>>> per-packet state is needed for some protocols, like Multipath
TCP.
>>>>>>
>>>>>> An optional shared control buffer placed after the normal struct
>>>>>> skb_shared_info can accomodate the necessary state without
>>>>>> imposing extra memory usage or code changes on normal struct
>>>>>> sk_buff users. __alloc_skb will now place a skb_shared_info_ext
>>>>>> structure at
>>>>>> skb->end when given the SKB_ALLOC_SHINFO_EXT flag. Most users
of
>>> struct
>>>>>> sk_buff continue to use the skb_shinfo() macro to access shared
>>>>>> info. skb_shinfo(skb)->is_ext is set if the extended
structure
>>>>>> is available, and cleared if it is not.
>>>>>>
>>>>>> pskb_expand_head will preserve the shared control buffer if it
is
>> present.
>>>>>>
>>>>>> Signed-off-by: Mat Martineau
>>>>>> <mathew.j.martineau(a)linux.intel.com>
>>>>>> ---
>>>>>> include/linux/skbuff.h | 24 +++++++++++++++++++++-
>>>>>> net/core/skbuff.c | 56
>> ++++++++++++++++++++++++++++++++++++++--
>>> -----
>>>>> -----
>>>>>> 2 files changed, 66 insertions(+), 14 deletions(-)
>>>>>
>>>>> Boris, below is the change I mentioned to you.
>>>>>
>>>>> It allows to allocate 48 additional bytes on-demand after
skb_shared_info.
>>>>> As it is on-demand, it won't increase the size of the skb for
other users.
>>>>>
>>>>> For example, TLS could start using it when it creates the skb that
>>>>> it pushes down to the TCP-stack. That way you don't need to
handle
>>>>> the tls_record lists.
>>>>>
>>>>
>>>> One small problem is that TLS doesn't create SKBs. As a ULP it calls
>>>> the
>>> transport send
>>>> Functions (do_tcp_sendpages for TLS). This function receives a page
>>>> and not a
>>> SKB.
>>>
>>> yes, that's a good point. Mat has another patch as part of this
>>> series, that adds an skb-arg to sendpages
>>>
>> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
>>> 01.org%2Fpipermail%2Fmptcp%2F2017-
>>>
>> October%2F000130.html&data=02%7C01%7Cborisp%40mellanox.com%7C8d
>> 0
>>>
>> 4f7b1c39649a6e80d08d5272d17fe%7Ca652971c7d2e4d9ba6a4d149256f461
>> b
>>>
>> %7C0%7C0%7C636457996996299024&sdata=YjycapXvL2N%2FkrPj15oTy2igh
>> C
>>> 23j1lKmWetdJtdXLU%3D&reserved=0)
>>>
>>> That should do the job for you.
>>>
>>>> We decided not to create the SKB outside of the TCP layer to reduce
>>>> the
>>> number of
>>>> changes we made to TCP.
>>>>
>>>> It would be nice if we could use something like that. Did you talk
>>>> to DaveM
>>> about
>>>> upstreaming this?
>>>
>>> No, we didn't talk yet to anyone outside of this list here about it.
>>>
>>> We were looking for a user of it outside of MPTCP but couldn't find one.
>>> Now, it seems like we found one that would be interested :)
>>>
>>>
>>> I think this infrastructure here would simplify your code quite a bit, no?
>>
>> I'm not sure. I'll need to think about that.
>>
>> I'm worried that in some cases it won't work, i.e. the shared_info
wouldn't
>> reach our driver and then again we would be forced to use the tls_get_record
>> function.
>> One example, is the tcp_mtu_probe function, which creates a new skb. Another
>> is tcp_collapse_retrans. We need to ensure all of these are covered when we
>> pass any offloaded TLS skb to the driver.
>>
>>>
>>>> We will definitely find it useful for the receive side, there we
>>>> allocate the SKB
>>> in the driver.
>>>
>>> Interesting! So even there you could use it. We were under the
>>> impression that it would be of less interest for the receive-side.
>>>
>>>
>>> Christoph
>>>
>>>>
>>>>> See below for the rest of the patch.
>>>>>
>>>>>
>>>>> Christoph
>>>>>
>
--
Mat Martineau
Intel OTC
1162
days inactive
1163
days old
1 comments
2 participants
participants (2)
-
cpaasch＠apple.com -
Mat Martineau