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

[Michael Breuer <mbreuer@majjas.com>]

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:
>>>>          
>>>>> David Miller wrote, On 12/27/2009 05:11 AM:
>>>>>
>>>>>
>>>>>            
>>>>>> From: David Miller<davem@davemloft.net>
>>>>>> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> From: Stephen Hemminger<shemminger@linux-foundation.org>
>>>>>>> Date: Sat, 26 Dec 2009 14:05:44 -0800
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>>>> Other drivers may have same problem, I really think this ought
>>>>>>>> to be done at higher level.
>>>>>>>>
>>>>>>>>                  
>>>>>>> I tend to agree with you, and I thought we had handled all
>>>>>>> cases.  Let's simply make AF_PACKET linearize the link
>>>>>>> level header before sending things out to the transmit path.
>>>>>>>
>>>>>>> I can work on this if you want.
>>>>>>>
>>>>>>>                
>>>>>> Actually Stephen, I took a look and I can't see how AF_PACKET
>>>>>> can create this situation.
>>>>>>
>>>>>> It always copies into the linear area of the SKB it allocates
>>>>>> for sendmsg() processing.  Whether the data comes from sendmsg
>>>>>> data or the mmap() ring buffer.
>>>>>>
>>>>>>              
>>>>> Actually, I think there is a bug in this place, but of course this
>>>>> might be unconnected. Anyway, Michael, could you try this patch?
>>>>> BTW, did you try with CONFIG_PACKET_MMAP disabled?
>>>>>
>>>>> Thanks,
>>>>> Jarek P.
>>>>> ----------------->
>>>>>
>>>>> 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);
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>>>
>>>>>            
>>>> This patch solves the original reported oops - as did Steven's patch of
>>>> 12/26: [PATCH] sky2: make sure ethernet header is in transmit skb (I ran
>>>> without Steven's patch and with this patch).
>>>>
>>>> Oddly, with this patch vs. Steven's - I'm getting software interrupt
>>>> errors sporadically while not under load - with Steven's I get the
>>>> frequently while under load (as per nethogs). For example:
>>>> Jan  5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt
>>>> status=0x40000008
>>>> Jan  5 21:29:00 mail kernel: sky2 software interrupt status 0x40000008
>>>> Jan  5 21:29:00 mail kernel: sky2 Tx ring pending=124...125 report=125
>>>> done=125
>>>> Jan  5 21:29:00 mail kernel: 124: 0xb38de0be(5374)
>>>> Jan  5 21:29:00 mail kernel: sky2 0000:06:00.0: error interrupt status=0x8
>>>> Jan  5 21:29:00 mail kernel: sky2 software interrupt status 0x8
>>>> Jan  5 21:29:00 mail kernel: sky2 Tx ring pending=126...127 report=126
>>>> done=127
>>>> Jan  5 21:29:00 mail kernel: 126: 0xb38d80be(9014)
>>>>
>>>> I also believe (can't prove yet) that my load test is slower with this
>>>> patch vs. the sky2 patch.
>>>>
>>>> Is it possible that this patch is causing the transmission to
>>>> momentarily halt such that the overall utilization appears low at the
>>>> time I see the interrupt errors, vs. the other patch which perhaps
>>>> doesn't interrupt the traffic flow quite so much?
>>>>          
>>> Yes, without this patch xmit could be stopped earlier on some kind of
>>> errors, with retransmit of the last message possible. On the other
>>> hand, other dev_queue_xmit() (negative) errors, are ignored. So this
>>> place could be still improved by adding proper err handling (or
>>> removing getting err return from dev_queue_xmit() at all).
>>>
>>> Anyway, I think this patch should be a safe proposal for stable. If
>>> so, David, please add Michael's "Tested-by".
>>>
>>>        
>>>> I haven't run yet with CONFIG_PACKET_MMAP disabled.
>>>>
>>>>          
>>> This should behave similarly as MMAP with this patch or maybe even
>>> better in case of errors.
>>>
>>> Thanks,
>>> Jarek P.
>>>        
>> 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.