Re: [PATCH net-next v2 10/10] amd-xgbe: Rework the Rx path SKB allocation

[Tom Lendacky <thomas.lendacky@amd.com>]

On 03/19/2015 05:07 PM, Tom Lendacky wrote:
> On 03/19/2015 04:51 PM, David Miller wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Date: Thu, 19 Mar 2015 15:54:24 -0500
>>
>>> On 03/19/2015 03:20 PM, David Miller wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Date: Thu, 19 Mar 2015 15:09:08 -0500
>>>>
>>>>> When the driver creates an SKB it currently only copies the header
>>>>> buffer data (which can be just the header if split header processing
>>>>> succeeded or header plus data if split header processing did not
>>>>> succeed) into the SKB. The receive buffer data is always added as a
>>>>> frag, even if it could fit in the SKB. As part of SKB creation, inline
>>>>> the receive buffer data if it will fit in the the SKB, otherwise add
>>>>> it
>>>>> as a frag during SKB creation.
>>>>>
>>>>> Also, Update the code to trigger off of the first/last descriptor
>>>>> indicators and remove the incomplete indicator.
>>>>>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> I do not understand the motivation for this, could you explain?
>>>>
>>>> The less copying you do the better, just having the headers in the
>>>> linear area is the most optimal situation, and have all the data
>>>> in page frag references.
>>>>
>>>
>>> I was trying to make the Rx path more logical from a first / last
>>> descriptor point of view.  If it's the first descriptor allocate the
>>> SKB, otherwise add the data as a frag. Compared to the current code:
>>> check for null skb pointer, allocate the SKB and if there's data left
>>> add it as a frag.
>>>
>>> I could keep the first / last descriptor methodology and in the
>>> xgbe_create_skb routine avoid the second copy and just always add the
>>> other buffer as a frag. That will eliminate the extra copying. Would
>>> that be ok or would you prefer that I just drop this patch?
>>
>> The point is, the data might not even be touched by the cpu so copying
>> it into the linear SKB data area could be a complete waste of cpu
>> cycles.
>
> I understood that point, sorry if I wasn't clear.  I'd would remove the
> copying of the data and just always add it as a frag in xgbe_create_skb
> routine so that only the headers are in the linear area.  What I'd like
> to do though is keep the overall changes of how I determine when to
> call the xgbe_create_skb routine so that it appears a bit cleaner,
> more logical.  The net effect is that the behavior of the code would
> remain the same (headers in the linear area, data as frags), but I feel
> it reads better and is easier to understand.

Actually, going back to the comment you made from one of the other
patches about programming defensively, I believe the current code is
safer since it will check for a NULL skb pointer and allocate one vs
this patch assuming the first descriptor bit will be set. Should the
hardware have a bug and not set that bit then the driver would try to
add a frag using a NULL skb pointer. I'm going to drop this patch in
the next version of the series I send out.

Thanks,
Tom

>
> Thanks,
> Tom
>
>>
>> Only the headers will be touched by the cpu in the packet processing
>> paths.
>>
>> And when we copy the packet data into userspace, that might even occur
>> on another cpu, so the data will just thrash between the individual
>> cpu's caches.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html