Re: [PATCH] af_packet: Don't use skb after dev_queue_xmit()

[Michael Breuer <mbreuer@majjas.com>]

On 1/6/2010 4:09 PM, Jarek Poplawski wrote:
> On Wed, Jan 06, 2010 at 03:33:05PM -0500, Michael Breuer wrote:
>    
>> On 1/6/2010 3:22 PM, Jarek Poplawski wrote:
>>      
>>> On Wed, Jan 06, 2010 at 02:49:38PM -0500, Michael Breuer wrote:
>>>        
>>>> On 1/6/2010 2:22 AM, Jarek Poplawski wrote:
>>>>          
>>>>> On Tue, Jan 05, 2010 at 09:36:28PM -0500, Michael Breuer wrote:
>>>>>            
>>>>>> On 1/5/2010 6:07 PM, Jarek Poplawski wrote:
>>>>>>              
>>>>>>> ----------------->
>>>>>>>
>>>>>>> Changing an skb after dev_queue_xmit() is illegal. And since it's
>>>>>>> inconsistent to treat specially net_xmit_errno() non-zero return,
>>>>>>> while ignoring other dev_queue_xmit() errors, there is no reason
>>>>>>> to break the loop in tpacket_snd() in this case.
>>>>>>>
>>>>>>> With debugging by: Stephen Hemminger<shemminger@linux-foundation.org>
>>>>>>>
>>>>>>> Reported-by: Michael Breuer<mbreuer@majjas.com>
>>>>>>> Signed-off-by: Jarek Poplawski<jarkao2@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>    net/packet/af_packet.c |    8 +++-----
>>>>>>>    1 files changed, 3 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>>>>>>> index e0516a2..984a1fa 100644
>>>>>>> --- a/net/packet/af_packet.c
>>>>>>> +++ b/net/packet/af_packet.c
>>>>>>> @@ -1021,8 +1021,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>>>>>>>
>>>>>>>    		status = TP_STATUS_SEND_REQUEST;
>>>>>>>    		err = dev_queue_xmit(skb);
>>>>>>> -		if (unlikely(err>     0&&     (err = net_xmit_errno(err)) != 0))
>>>>>>> -			goto out_xmit;
>>>>>>> +		if (unlikely(err>     0))
>>>>>>> +			err = net_xmit_errno(err);
>>>>>>> +
>>>>>>>    		packet_increment_head(&po->tx_ring);
>>>>>>>    		len_sum += tp_len;
>>>>>>>    	} while (likely((ph != NULL) ||
>>>>>>> @@ -1033,9 +1034,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>>>>>>>    	err = len_sum;
>>>>>>>    	goto out_put;
>>>>>>>
>>>>>>> -out_xmit:
>>>>>>> -	skb->destructor = sock_wfree;
>>>>>>> -	atomic_dec(&po->tx_ring.pending);
>>>>>>>    out_status:
>>>>>>>    	__packet_set_status(po, ph, status);
>>>>>>>    	kfree_skb(skb);
>>>>>>> --
>>>>>>>                
> ...
>    
>>>> This patch at first behaved similarly to the previous one - seemed
>>>> to be running a bit better... until the adapter went down :(
>>>>          
>>> I'm not sure: do you mean this patch above vs previous one by Stephen,
>>> or did you manage to try my "alernative #2" patch already?
>>>
>>> BTW, I forgot to mention, and maybe it doesn't matter here, but it
>>> would be better to (always) use my sky2 patch from Berck Nash's
>>> thread.
>>>
>>> Jarek P.
>>>        
>> This was using "alternative #2" patch. I didn't get the hang with
>> alternative #1. Your sky2 patch from Berck Nash's thread was
>> included in both cases; Stephen's was not.
>>      
> OK, so I guess "alternative #1" (above) seems safer to recommend for
> now (as I assumed earlier).
>
> On the other hand, we really don't know if it's only because it's
> because it's nicer for your hardware (or still some other bug around),
> so as before: let David choose ;-)
>
> BTW, I think you could still use Stephen's patch too (there might be
> still something more like this). There was also mentioned this network
> manager again. I might be wrong, but IMHO there could be some
> interaction even if it doesn't use this device; so could/did you try
> to disable it entirely?
>
> Thanks for testing!
> Jarek P.
>
>
>    
Just reran without the network manager - no change. Going to rerun with 
Stephen's new patch, alternative #1, and the patch from Berck Nash's thread.