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

[Michael Breuer]

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@xxxxxxxxxxxxxxxxxxxx>
> > > > > > >
> > > > > > > Reported-by: Michael Breuer<mbreuer@xxxxxxxxxx>
> > > > > > > Signed-off-by: Jarek Poplawski<jarkao2@xxxxxxxxx>
> > > > > > > ---
> > > > > > >
> > > > > > >    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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/