Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods

Ben Greear
2011-07-21T22:26:11Z

On 07/21/2011 03:19 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Thu, 21 Jul 2011 15:14:32 -0700
>
>> On 07/21/2011 03:01 PM, David Miller wrote:
>>> From: Neil Horman<nhorman@tuxdriver.com>
>>> Date: Wed, 20 Jul 2011 11:18:27 -0400
>>>
>>>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:
>>>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :
>>>>>>>
>>>>>> I think this is a good idea. It lets pktgen dynamically make the
>>>>>> clone/share
>>>>>> decision dynamically and only impacts performance for those systems.
>>>>>>
>>>>>
>>>>> Just let pktgen refuse to use clone_skb command for these devices.
>>>>>
>>>> copy that, This is by no means final, but what do you think of this?
>>>> If its
>>>> agreeable to you, Ben, et al. I can add this to my local tree and
>>>> start auditing
>>>> all the drivers that may need to have the flag set.
>>>
>>> I think there is a much simpler solution.
>>>
>>> Set a flag in the SKB when pktgen does SKB sharing.
>>>
>>> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the
>>> flag
>>> and if it's set then we copy the SKB.
>>>
>>> If this works, then we fix the crash and no driver changes are
>>> necessary both now and in the future.
>>
>> Doesn't that make clone-skb in pktgen much less efficient
>> in all cases?
>
> No, the copy only happens if we enter dev_queue_xmit() which pktgen
> doesn't do, it calls the driver's ->ndo_start_xmit() method directly.
>
> That's the whole idea. Only these encapsulating software devices
> will trigger the condition.

It seems there may be some Ethernet drivers that have similar issues,
though I don't know of any personally (many years ago, Chelsio 10G NICs had
this issue, but it may be fixed now.)

But, it should be no worse than what we have today, and now that I
better understand what you suggest, it sounds OK to me.

Thanks,
Ben

--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com