kern/86306: [patch] if_em.c locks up while trying to send a highly fragmented packet

Mon Sep 19 00:40:20 PDT 2005

The following reply was made to PR kern/86306; it has been noted by GNATS.

From: Dmitrij Tejblum <tejblum at yandex-team.ru>
To: Ruslan Ermilov <ru at freebsd.org>
Cc: FreeBSD-gnats-submit at freebsd.org
Subject: Re: kern/86306: [patch] if_em.c locks up while trying to send a highly
 fragmented packet
Date: Mon, 19 Sep 2005 11:38:47 +0400

 This is a multi-part message in MIME format.
 --------------080903090009080801040205
 Content-Type: text/plain; charset=ISO-8859-1; format=flowed
 Content-Transfer-Encoding: 7bit

 Ruslan Ermilov wrote:

 >Hi Dmitrij,
 >
 >On Sun, Sep 18, 2005 at 11:25:35PM +0400, Dmitrij Tejblum wrote:
 >  
 >
 >>When em_encap() tries to send a very long mbuf chain (i.e. more than
 >>EM_MAX_SCATTER == 64 mbufs), bus_dmamap_load_mbuf_sg() may fail with EFBIG. 
 >>Then em_encap() fail, the packet is not sent and left in the output queue, 
 >>and thus no futher transmission is possible.
 >>
 >>Some other driver handle similar condition with m_defrag(9) function
 >>(which is intended for this purpose).
 >>
 >>    
 >>
 >Can you please modify your patch as follows:
 >
 >1) Count how much fragments are in the packet in em_encap() first, and
 >   do m_defrag() if it exceeeds EM_MAX_SCATTER, like in if_dc.c.  If it
 >   is still EFBIG after that and bus_dmamap_load_mbuf_sg(), then free it
 >   as you do to prevent re-enqueue.
 >  
 >
 So you think that if_dc.c is a better example than e.g. if_fxp.c and 
 if_xl.c? Why? I also suspect that your suggestion would be a pessimisation.

 >2) Put BPF processing back to em_start_locked().
 >  
 >
 As I wrote, I think that if e.g. we were unable to defragment a packet 
 it is better to drop it and try to send the next, rather than stop and 
 set OACTIVE (since the code that clear OACTIVE assume that it is about 
 TX descriptors). (You didn't suggest to change that).  I moved BPF 
 processing since it would not be a good idea to pass NULL to BPF.

 >3) Pull up to the HEAD version of the driver.
 >  
 >
 Well, please answer 1 and 2 first :-).

 >
 >Cheers,
 >  
 >

 --------------080903090009080801040205
 Content-Type: text/html; charset=ISO-8859-1
 Content-Transfer-Encoding: 7bit

 <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
 <html>
 <head>
   <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
 </head>
 <body bgcolor="#ffffff" text="#000000">
 Ruslan Ermilov wrote:
 <blockquote cite="mid20050919062926.GF65954 at ip.net.ua" type="cite">
   <pre wrap="">Hi Dmitrij,

 On Sun, Sep 18, 2005 at 11:25:35PM +0400, Dmitrij Tejblum wrote:
   </pre>
   <blockquote type="cite">
     <pre wrap="">When em_encap() tries to send a very long mbuf chain (i.e. more than
 EM_MAX_SCATTER == 64 mbufs), bus_dmamap_load_mbuf_sg() may fail with EFBIG. 
 Then em_encap() fail, the packet is not sent and left in the output queue, 
 and thus no futher transmission is possible.

 Some other driver handle similar condition with m_defrag(9) function
 (which is intended for this purpose).

     </pre>
   </blockquote>
   <pre wrap=""><!---->Can you please modify your patch as follows:

 1) Count how much fragments are in the packet in em_encap() first, and
    do m_defrag() if it exceeeds EM_MAX_SCATTER, like in if_dc.c.  If it
    is still EFBIG after that and bus_dmamap_load_mbuf_sg(), then free it
    as you do to prevent re-enqueue.
   </pre>
 </blockquote>
 So you think that if_dc.c is a better example than e.g. if_fxp.c and
 if_xl.c? Why? I also suspect that your suggestion would be a
 pessimisation.<br>
 <blockquote cite="mid20050919062926.GF65954 at ip.net.ua" type="cite">
   <pre wrap="">
 2) Put BPF processing back to em_start_locked().
   </pre>
 </blockquote>
 As I wrote, I think that if e.g. we were unable to defragment a packet
 it is better to drop it and try to send the next, rather than stop and
 set OACTIVE (since the code that clear OACTIVE assume that it is about
 TX descriptors). (You didn't suggest to change that).&nbsp; I moved BPF
 processing since it would not be a good idea to pass NULL to BPF.<br>
 <blockquote cite="mid20050919062926.GF65954 at ip.net.ua" type="cite">
   <pre wrap="">
 3) Pull up to the HEAD version of the driver.
   </pre>
 </blockquote>
 Well, please answer 1 and 2 first :-).<br>
 <blockquote cite="mid20050919062926.GF65954 at ip.net.ua" type="cite">
   <pre wrap="">

 Cheers,
   </pre>
 </blockquote>
 <br>
 </body>
 </html>

 --------------080903090009080801040205--