Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header,Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header
From: Doug Ledford
Date: Thu, 13 Oct 2016 10:35:35 -0400
> On 10/13/2016 10:24 AM, David Miller wrote:
>> From: Paolo Abeni
>> Date: Tue, 11 Oct 2016 19:15:44 +0200
>>
>>> After the commit 9207f9d45b0a ("net: preserve IP control block
>>> during GSO segmentation"), the GSO CB and the IPoIB CB conflict.
>>> That destroy the IPoIB address information cached there,
>>> causing a severe performance regression, as better described here:
>>>
>>> http://marc.info/?l=linux-kernel&m=146787279825501&w=2
>>>
>>> This change moves the data cached by the IPoIB driver from the
>>> skb control lock into the IPoIB hard header, as done before
>>> the commit 936d7de3d736 ("IPoIB: Stop lying about hard_header_len
>>> and use skb->cb to stash LL addresses").
>>> In order to avoid GRO issue, on packet reception, the IPoIB driver
>>> stash into the skb a dummy pseudo header, so that the received
>>> packets have actually a hard header matching the declared length.
>>> Also the connected mode maximum mtu is reduced by 16 bytes to
>>> cope with the increased hard header len.
>>>
>>> After this commit, IPoIB performances are back to pre-regression
>>> value.
>>>
>>> Fixes: 9207f9d45b0a ("net: preserve IP control block during GSO
>>> segmentation")
>>> Signed-off-by: Paolo Abeni
>>
>> Not providing an accurate hard_header_len causes many problems.
>>
>> In fact we recently fixed the mlxsw driver to stop doing this.
>>
>
> Sure, but there are too many users of the cb struct, and whatever
> problems you are saying there are by lying about the hard header len are
> dwarfed by the problems caused by the inability to store the ll address
> anywhere between hard_header and send time.
IB wants to pass addressing information between layers, it needs to
find a safe way to do that. The currently propsoed patch does not
meet this criteria.
Pushing metadata before the head of the SKB data pointer is illegal,
as the layers in between might want to push protocol headers, mirror
the packet to another interface, etc.
So this "metadata in SKB data" approach is buggy too.
Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header,Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header
On 10/13/2016 10:43 AM, David Miller wrote:
> From: Doug Ledford
> Date: Thu, 13 Oct 2016 10:35:35 -0400
>
>> On 10/13/2016 10:24 AM, David Miller wrote:
>>> From: Paolo Abeni
>>> Date: Tue, 11 Oct 2016 19:15:44 +0200
>>>
After the commit 9207f9d45b0a ("net: preserve IP control block
during GSO segmentation"), the GSO CB and the IPoIB CB conflict.
That destroy the IPoIB address information cached there,
causing a severe performance regression, as better described here:
http://marc.info/?l=linux-kernel&m=146787279825501&w=2
This change moves the data cached by the IPoIB driver from the
skb control lock into the IPoIB hard header, as done before
the commit 936d7de3d736 ("IPoIB: Stop lying about hard_header_len
and use skb->cb to stash LL addresses").
In order to avoid GRO issue, on packet reception, the IPoIB driver
stash into the skb a dummy pseudo header, so that the received
packets have actually a hard header matching the declared length.
Also the connected mode maximum mtu is reduced by 16 bytes to
cope with the increased hard header len.
After this commit, IPoIB performances are back to pre-regression
value.
Fixes: 9207f9d45b0a ("net: preserve IP control block during GSO
segmentation")
Signed-off-by: Paolo Abeni
>>>
>>> Not providing an accurate hard_header_len causes many problems.
>>>
>>> In fact we recently fixed the mlxsw driver to stop doing this.
>>>
>>
>> Sure, but there are too many users of the cb struct, and whatever
>> problems you are saying there are by lying about the hard header len are
>> dwarfed by the problems caused by the inability to store the ll address
>> anywhere between hard_header and send time.
>
> IB wants to pass addressing information between layers, it needs to
> find a safe way to do that.
We *had* a safe way to do that. It got broken. What about increasing
the size of skb->cb? Or adding a skb->dgid that is a
u8[INFINIBAND_ALEN]? Or a more generic skb->dest_ll_addr that is sized
to hold the dest address for any link layer?
> Pushing metadata before the head of the SKB data pointer is illegal,
> as the layers in between might want to push protocol headers,
That's a total non-issue for us. There are no headers that protocols
can add before ours.
> mirror
> the packet to another interface
Doesn't that then mean that the headers specific to this interface
should be stripped before the mirror? If so, I believe the way Paolo
did this patch, that is what will be done.
>, etc.
>
> So this "metadata in SKB data" approach is buggy too.
--
Doug Ledford
GPG Key ID: 0E572FDD
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header,Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header,Re: [PATCH] IB/ipoib: move back the IB LL address into the
From: Doug Ledford Date: Thu, 13 Oct 2016 11:20:59 -0400 > We *had* a safe way to do that. It got broken. What about increasing > the size of skb->cb? Or adding a skb->dgid that is a > u8[INFINIBAND_ALEN]? Or a more generic skb->dest_ll_addr that is sized > to hold the dest address for any link layer? I understand the situation, and I also believe that making sk_buff any huger than it already is happens to be a non-starter. >> Pushing metadata before the head of the SKB data pointer is illegal, >> as the layers in between might want to push protocol headers, > > That's a total non-issue for us. There are no headers that protocols > can add before ours. Ok, if that's the case, and based upon Paolo's response to me it appears to be, I guess this is OK for now. Paolo please resubmit your patch, thanks.
