Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Neil Horman <nhorman@tuxdriver.com>
Date: 2011-07-21 23:51:03
On Thu, Jul 21, 2011 at 03:19:03PM -0700, David Miller wrote:
From: Ben Greear <redacted> Date: Thu, 21 Jul 2011 15:14:32 -0700quoted
On 07/21/2011 03:01 PM, David Miller wrote:quoted
From: Neil Horman<nhorman@tuxdriver.com> Date: Wed, 20 Jul 2011 11:18:27 -0400quoted
On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote:quoted
Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit :quoted
quoted
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.
I'm happy to go down this route Dave, and agree, its a more solid solution, but I think the problem with it (which Ben may have been alluding to previously) is that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames. It mimics the locking of dev_hard_start_xmit (but ignores the other checks that function does), and just calls the ndo_start_xmit routine of the driver directly. So theres no common code that an skb traverses from pktgen to a given driver where we can check such a flag Again, I'm happy to change that so that pktgen uses dev_hard_start_xmit, but I wonder if thats going to get the same sort of pushback about performance that my origional patch did. Eric, et al., thoughts? Regards Neil