Current-Users archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: BNX Double Buffer Allocation



On Wed, Aug 20, 2008 at 07:26:21PM -0500, David Dudley wrote:
> Just updated -current and installed a new kernel on Sunday.
> 
> Installed on a dual-quad core DELL 1950.  Incredibly fast machine, but
> I can only use one of the network interfaces.
> 
> I take it there hasn't been any movement on correcting the problem with
> the double buffer allocation in the bnx(4) driver?
> 
> Still get a system crash in a few seconds.

David,

It looks to me like bnx_get_buf(), which tries to refill the rx buffer
chain almost to capacity, is used inappropriately on the double-buffer
error path.

On the double-buffer error path, mbufs have run out.  bnx_get_buf()
is handed back an mbuf that just came off of the rx chain, to put
back onto the chain.  After it has put that mbuf back onto the chain,
it may try to allocate another mbuf, in order to help fill the chain.
Since that fails, there is a panic.

My solution is to extract a subroutine from bnx_get_buf(), bnx_add_buf(),
that does the very least work that is necessary to place an rx buffer
back on the chain.  I use bnx_add_buf() instead of bnx_get_buf() to try
to recover from a failure of bnx_get_buf().

I have attached a patch.  It compiles, but I have not tested it, because
my Dell 1950 is not plugged in these days.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933 ext 24
Index: sys/dev/pci/if_bnx.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_bnx.c,v
retrieving revision 1.18
diff -p -u -u -p -r1.18 if_bnx.c
--- sys/dev/pci/if_bnx.c        7 Feb 2008 01:21:55 -0000       1.18
+++ sys/dev/pci/if_bnx.c        22 Aug 2008 22:06:58 -0000
@@ -316,8 +316,9 @@ void        bnx_stop(struct ifnet *, int);
 int    bnx_reset(struct bnx_softc *, u_int32_t);
 int    bnx_chipinit(struct bnx_softc *);
 int    bnx_blockinit(struct bnx_softc *);
-int    bnx_get_buf(struct bnx_softc *, struct mbuf *, u_int16_t *,
+static int     bnx_add_buf(struct bnx_softc *, struct mbuf *, u_int16_t *,
            u_int16_t *, u_int32_t *);
+int    bnx_get_buf(struct bnx_softc *, u_int16_t *, u_int16_t *, u_int32_t *);
 
 int    bnx_init_tx_chain(struct bnx_softc *);
 int    bnx_init_rx_chain(struct bnx_softc *);
@@ -3018,6 +3019,107 @@ bnx_blockinit_exit:
        return (rc);
 }
 
+static int
+bnx_add_buf(struct bnx_softc *sc, struct mbuf *m_new, u_int16_t *prod,
+    u_int16_t *chain_prod, u_int32_t *prod_bseq)
+{
+       bus_dmamap_t            map;
+       struct rx_bd            *rxbd;
+       u_int32_t               addr;
+       int i;
+#ifdef BNX_DEBUG
+       u_int16_t debug_chain_prod =    *chain_prod;
+#endif
+       u_int16_t first_chain_prod;
+
+       m_new->m_len = m_new->m_pkthdr.len = sc->mbuf_alloc_size;
+
+       /* Map the mbuf cluster into device memory. */
+       map = sc->rx_mbuf_map[*chain_prod];
+       first_chain_prod = *chain_prod;
+       if (bus_dmamap_load_mbuf(sc->bnx_dmatag, map, m_new, BUS_DMA_NOWAIT)) {
+               BNX_PRINTF(sc, "%s(%d): Error mapping mbuf into RX chain!\n",
+                   __FILE__, __LINE__);
+
+               m_freem(m_new);
+
+               DBRUNIF(1, sc->rx_mbuf_alloc--);
+
+               return ENOBUFS;
+       }
+       bus_dmamap_sync(sc->bnx_dmatag, map, 0, map->dm_mapsize,
+           BUS_DMASYNC_PREREAD);
+
+       /* Watch for overflow. */
+       DBRUNIF((sc->free_rx_bd > USABLE_RX_BD),
+           aprint_error_dev(sc->bnx_dev,
+               "Too many free rx_bd (0x%04X > 0x%04X)!\n",
+               sc->free_rx_bd, (u_int16_t)USABLE_RX_BD));
+
+       DBRUNIF((sc->free_rx_bd < sc->rx_low_watermark), 
+           sc->rx_low_watermark = sc->free_rx_bd);
+
+       /*
+        * Setup the rx_bd for the first segment
+        */
+       rxbd = &sc->rx_bd_chain[RX_PAGE(*chain_prod)][RX_IDX(*chain_prod)];
+
+       addr = (u_int32_t)(map->dm_segs[0].ds_addr);
+       rxbd->rx_bd_haddr_lo = htole32(addr);
+       addr = (u_int32_t)((u_int64_t)map->dm_segs[0].ds_addr >> 32);
+       rxbd->rx_bd_haddr_hi = htole32(addr);
+       rxbd->rx_bd_len = htole32(map->dm_segs[0].ds_len);
+       rxbd->rx_bd_flags = htole32(RX_BD_FLAGS_START);
+       *prod_bseq += map->dm_segs[0].ds_len;
+       bus_dmamap_sync(sc->bnx_dmatag,
+           sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
+           sizeof(struct rx_bd) * RX_IDX(*chain_prod), sizeof(struct rx_bd),
+           BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
+
+       for (i = 1; i < map->dm_nsegs; i++) {
+               *prod = NEXT_RX_BD(*prod);
+               *chain_prod = RX_CHAIN_IDX(*prod); 
+
+               rxbd =
+                   &sc->rx_bd_chain[RX_PAGE(*chain_prod)][RX_IDX(*chain_prod)];
+
+               addr = (u_int32_t)(map->dm_segs[i].ds_addr);
+               rxbd->rx_bd_haddr_lo = htole32(addr);
+               addr = (u_int32_t)((u_int64_t)map->dm_segs[i].ds_addr >> 32);
+               rxbd->rx_bd_haddr_hi = htole32(addr);
+               rxbd->rx_bd_len = htole32(map->dm_segs[i].ds_len);
+               rxbd->rx_bd_flags = 0;
+               *prod_bseq += map->dm_segs[i].ds_len;
+               bus_dmamap_sync(sc->bnx_dmatag,
+                   sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
+                   sizeof(struct rx_bd) * RX_IDX(*chain_prod),
+                   sizeof(struct rx_bd), BUS_DMASYNC_PREREAD | 
BUS_DMASYNC_PREWRITE);
+       }
+
+       rxbd->rx_bd_flags |= htole32(RX_BD_FLAGS_END);
+       bus_dmamap_sync(sc->bnx_dmatag,
+           sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
+           sizeof(struct rx_bd) * RX_IDX(*chain_prod),
+           sizeof(struct rx_bd), BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
+
+       /*
+        * Save the mbuf, ajust the map pointer (swap map for first and
+        * last rx_bd entry to that rx_mbuf_ptr and rx_mbuf_map matches)
+        * and update counter.
+        */
+       sc->rx_mbuf_ptr[*chain_prod] = m_new;
+       sc->rx_mbuf_map[first_chain_prod] = sc->rx_mbuf_map[*chain_prod];
+       sc->rx_mbuf_map[*chain_prod] = map;
+       sc->free_rx_bd -= map->dm_nsegs;
+
+       DBRUN(BNX_VERBOSE_RECV, bnx_dump_rx_mbuf_chain(sc, debug_chain_prod, 
+           map->dm_nsegs));
+       *prod = NEXT_RX_BD(*prod);
+       *chain_prod = RX_CHAIN_IDX(*prod); 
+
+       return 0;
+}
+
 /****************************************************************************/
 /* Encapsulate an mbuf cluster into the rx_bd chain.                        */
 /*                                                                          */
@@ -3029,18 +3131,11 @@ bnx_blockinit_exit:
 /*   0 for success, positive value for failure.                             */
 /****************************************************************************/
 int
-bnx_get_buf(struct bnx_softc *sc, struct mbuf *m, u_int16_t *prod,
+bnx_get_buf(struct bnx_softc *sc, u_int16_t *prod,
     u_int16_t *chain_prod, u_int32_t *prod_bseq)
 {
-       bus_dmamap_t            map;
        struct mbuf             *m_new = NULL;
-       struct rx_bd            *rxbd;
-       int                     i, rc = 0;
-       u_int32_t               addr;
-#ifdef BNX_DEBUG
-       u_int16_t debug_chain_prod =    *chain_prod;
-#endif
-       u_int16_t first_chain_prod;
+       int                     rc = 0;
        u_int16_t min_free_bd;
 
        DBPRINT(sc, (BNX_VERBOSE_RESET | BNX_VERBOSE_RECV), "Entering %s()\n", 
@@ -3062,137 +3157,49 @@ bnx_get_buf(struct bnx_softc *sc, struct
        else
                min_free_bd = (BNX_MAX_MRU + PAGE_SIZE - 1) / PAGE_SIZE;
        while (sc->free_rx_bd >= min_free_bd) {
-               if (m == NULL) {
-                       
DBRUNIF(DB_RANDOMTRUE(bnx_debug_mbuf_allocation_failure),
-                           BNX_PRINTF(sc, "Simulating mbuf allocation 
failure.\n");
-
-                               sc->mbuf_alloc_failed++;
-                               rc = ENOBUFS;
-                               goto bnx_get_buf_exit);
-
-                       /* This is a new mbuf allocation. */
-                       MGETHDR(m_new, M_DONTWAIT, MT_DATA);
-                       if (m_new == NULL) {
-                               DBPRINT(sc, BNX_WARN,
-                                   "%s(%d): RX mbuf header allocation 
failed!\n", 
-                                   __FILE__, __LINE__);
+               DBRUNIF(DB_RANDOMTRUE(bnx_debug_mbuf_allocation_failure),
+                   BNX_PRINTF(sc, "Simulating mbuf allocation failure.\n");
 
-                               DBRUNIF(1, sc->mbuf_alloc_failed++);
+                       sc->mbuf_alloc_failed++;
+                       rc = ENOBUFS;
+                       goto bnx_get_buf_exit);
 
-                               rc = ENOBUFS;
-                               goto bnx_get_buf_exit;
-                       }
+               /* This is a new mbuf allocation. */
+               MGETHDR(m_new, M_DONTWAIT, MT_DATA);
+               if (m_new == NULL) {
+                       DBPRINT(sc, BNX_WARN,
+                           "%s(%d): RX mbuf header allocation failed!\n", 
+                           __FILE__, __LINE__);
 
-                       DBRUNIF(1, sc->rx_mbuf_alloc++);
-                       if (sc->mbuf_alloc_size == MCLBYTES)
-                               MCLGET(m_new, M_DONTWAIT);
-                       else
-                               MEXTMALLOC(m_new, sc->mbuf_alloc_size,
-                                   M_DONTWAIT);
-                       if (!(m_new->m_flags & M_EXT)) {
-                               DBPRINT(sc, BNX_WARN,
-                                   "%s(%d): RX mbuf chain allocation 
failed!\n", 
-                                   __FILE__, __LINE__);
-                               
-                               m_freem(m_new);
+                       DBRUNIF(1, sc->mbuf_alloc_failed++);
 
-                               DBRUNIF(1, sc->rx_mbuf_alloc--);
-                               DBRUNIF(1, sc->mbuf_alloc_failed++);
+                       rc = ENOBUFS;
+                       goto bnx_get_buf_exit;
+               }
 
-                               rc = ENOBUFS;
-                               goto bnx_get_buf_exit;
-                       }
-                               
-               } else {
-                       m_new = m;
-                       m = NULL;
-                       m_new->m_data = m_new->m_ext.ext_buf;
-               }
-               m_new->m_len = m_new->m_pkthdr.len = sc->mbuf_alloc_size;
-
-               /* Map the mbuf cluster into device memory. */
-               map = sc->rx_mbuf_map[*chain_prod];
-               first_chain_prod = *chain_prod;
-               if (bus_dmamap_load_mbuf(sc->bnx_dmatag, map, m_new, 
BUS_DMA_NOWAIT)) {
-                       BNX_PRINTF(sc, "%s(%d): Error mapping mbuf into RX 
chain!\n",
+               DBRUNIF(1, sc->rx_mbuf_alloc++);
+               if (sc->mbuf_alloc_size == MCLBYTES)
+                       MCLGET(m_new, M_DONTWAIT);
+               else
+                       MEXTMALLOC(m_new, sc->mbuf_alloc_size,
+                           M_DONTWAIT);
+               if (!(m_new->m_flags & M_EXT)) {
+                       DBPRINT(sc, BNX_WARN,
+                           "%s(%d): RX mbuf chain allocation failed!\n", 
                            __FILE__, __LINE__);
-
+                       
                        m_freem(m_new);
 
                        DBRUNIF(1, sc->rx_mbuf_alloc--);
+                       DBRUNIF(1, sc->mbuf_alloc_failed++);
 
                        rc = ENOBUFS;
                        goto bnx_get_buf_exit;
                }
-               bus_dmamap_sync(sc->bnx_dmatag, map, 0, map->dm_mapsize,
-                   BUS_DMASYNC_PREREAD);
-
-               /* Watch for overflow. */
-               DBRUNIF((sc->free_rx_bd > USABLE_RX_BD),
-                   aprint_error_dev(sc->bnx_dev,
-                       "Too many free rx_bd (0x%04X > 0x%04X)!\n",
-                       sc->free_rx_bd, (u_int16_t)USABLE_RX_BD));
-
-               DBRUNIF((sc->free_rx_bd < sc->rx_low_watermark), 
-                   sc->rx_low_watermark = sc->free_rx_bd);
-
-               /*
-                * Setup the rx_bd for the first segment
-                */
-               rxbd = 
&sc->rx_bd_chain[RX_PAGE(*chain_prod)][RX_IDX(*chain_prod)];
-
-               addr = (u_int32_t)(map->dm_segs[0].ds_addr);
-               rxbd->rx_bd_haddr_lo = htole32(addr);
-               addr = (u_int32_t)((u_int64_t)map->dm_segs[0].ds_addr >> 32);
-               rxbd->rx_bd_haddr_hi = htole32(addr);
-               rxbd->rx_bd_len = htole32(map->dm_segs[0].ds_len);
-               rxbd->rx_bd_flags = htole32(RX_BD_FLAGS_START);
-               *prod_bseq += map->dm_segs[0].ds_len;
-               bus_dmamap_sync(sc->bnx_dmatag,
-                   sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
-                   sizeof(struct rx_bd) * RX_IDX(*chain_prod), sizeof(struct 
rx_bd),
-                   BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
-
-               for (i = 1; i < map->dm_nsegs; i++) {
-                       *prod = NEXT_RX_BD(*prod);
-                       *chain_prod = RX_CHAIN_IDX(*prod); 
-
-                       rxbd =
-                           
&sc->rx_bd_chain[RX_PAGE(*chain_prod)][RX_IDX(*chain_prod)];
-
-                       addr = (u_int32_t)(map->dm_segs[i].ds_addr);
-                       rxbd->rx_bd_haddr_lo = htole32(addr);
-                       addr = (u_int32_t)((u_int64_t)map->dm_segs[i].ds_addr 
>> 32);
-                       rxbd->rx_bd_haddr_hi = htole32(addr);
-                       rxbd->rx_bd_len = htole32(map->dm_segs[i].ds_len);
-                       rxbd->rx_bd_flags = 0;
-                       *prod_bseq += map->dm_segs[i].ds_len;
-                       bus_dmamap_sync(sc->bnx_dmatag,
-                           sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
-                           sizeof(struct rx_bd) * RX_IDX(*chain_prod),
-                           sizeof(struct rx_bd), BUS_DMASYNC_PREREAD | 
BUS_DMASYNC_PREWRITE);
-               }
-
-               rxbd->rx_bd_flags |= htole32(RX_BD_FLAGS_END);
-               bus_dmamap_sync(sc->bnx_dmatag,
-                   sc->rx_bd_chain_map[RX_PAGE(*chain_prod)],
-                   sizeof(struct rx_bd) * RX_IDX(*chain_prod),
-                   sizeof(struct rx_bd), BUS_DMASYNC_PREREAD | 
BUS_DMASYNC_PREWRITE);
-
-               /*
-                * Save the mbuf, ajust the map pointer (swap map for first and
-                * last rx_bd entry to that rx_mbuf_ptr and rx_mbuf_map matches)
-                * and update counter.
-                */
-               sc->rx_mbuf_ptr[*chain_prod] = m_new;
-               sc->rx_mbuf_map[first_chain_prod] = 
sc->rx_mbuf_map[*chain_prod];
-               sc->rx_mbuf_map[*chain_prod] = map;
-               sc->free_rx_bd -= map->dm_nsegs;
-
-               DBRUN(BNX_VERBOSE_RECV, bnx_dump_rx_mbuf_chain(sc, 
debug_chain_prod, 
-                   map->dm_nsegs));
-               *prod = NEXT_RX_BD(*prod);
-               *chain_prod = RX_CHAIN_IDX(*prod); 
+                       
+               rc = bnx_add_buf(sc, m_new, prod, chain_prod, prod_bseq);
+               if (rc != 0)
+                       goto bnx_get_buf_exit;
        }
 
 bnx_get_buf_exit:
@@ -3384,7 +3391,7 @@ bnx_init_rx_chain(struct bnx_softc *sc)
        /* Allocate mbuf clusters for the rx_bd chain. */
        prod = prod_bseq = 0;
        chain_prod = RX_CHAIN_IDX(prod);
-       if (bnx_get_buf(sc, NULL, &prod, &chain_prod, &prod_bseq)) {
+       if (bnx_get_buf(sc, &prod, &chain_prod, &prod_bseq)) {
                BNX_PRINTF(sc,
                    "Error filling RX chain: rx_bd[0x%04X]!\n", chain_prod);
        }
@@ -3650,7 +3657,7 @@ bnx_rx_intr(struct bnx_softc *sc)
                                DBRUNIF(1, sc->l2fhdr_status_errors++);
 
                                /* Reuse the mbuf for a new frame. */
-                               if (bnx_get_buf(sc, m, &sw_prod,
+                               if (bnx_add_buf(sc, m, &sw_prod,
                                    &sw_chain_prod, &sw_prod_bseq)) {
                                        DBRUNIF(1, bnx_breakpoint(sc));
                                        panic("%s: Can't reuse RX mbuf!\n",
@@ -3665,7 +3672,7 @@ bnx_rx_intr(struct bnx_softc *sc)
                         * log an ierror on the interface, and generate
                         * an error in the system log.
                         */
-                       if (bnx_get_buf(sc, NULL, &sw_prod, &sw_chain_prod,
+                       if (bnx_get_buf(sc, &sw_prod, &sw_chain_prod,
                            &sw_prod_bseq)) {
                                DBRUN(BNX_WARN, BNX_PRINTF(sc, "Failed to 
allocate "
                                        "new mbuf, incoming frame dropped!\n"));
@@ -3673,7 +3680,7 @@ bnx_rx_intr(struct bnx_softc *sc)
                                ifp->if_ierrors++;
 
                                /* Try and reuse the exisitng mbuf. */
-                               if (bnx_get_buf(sc, m, &sw_prod,
+                               if (bnx_add_buf(sc, m, &sw_prod,
                                    &sw_chain_prod, &sw_prod_bseq)) {
                                        DBRUNIF(1, bnx_breakpoint(sc));
                                        panic("%s: Double mbuf allocation "
@@ -4834,7 +4843,7 @@ bnx_tick(void *xsc)
        prod = sc->rx_prod;
        prod_bseq = sc->rx_prod_bseq;
        chain_prod = RX_CHAIN_IDX(prod);
-       bnx_get_buf(sc, NULL, &prod, &chain_prod, &prod_bseq);
+       bnx_get_buf(sc, &prod, &chain_prod, &prod_bseq);
        sc->rx_prod = prod;
        sc->rx_prod_bseq = prod_bseq;
        splx(s);


Home | Main Index | Thread Index | Old Index