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

From: dfil...@freebsd.org (dfilter service)
To: bug-follo...@freebsd.org
Cc:  
Subject: Re: i386/134926: commit references a PR
Date: Wed, 27 May 2009 02:49:22 +0000 (UTC)

 Author: adrian
 Date: Wed May 27 02:49:08 2009
 New Revision: 192871
 URL: http://svn.freebsd.org/changeset/base/192871
 
 Log:
   Ensure that there are enough TX mbuf ring slots available before beginning
   to dequeue a packet.
   
   The tx path was trying to ensure that enough Xenbus TX ring slots existed but
   it didn't check to see whether the mbuf TX ring slots were also available.
   They get freed in xn_txeof() which occurs after transmission, rather than 
earlier
   on in the process. (The same happens under Linux too.)
   
   Due to whatever reason (CPU use, scheduling, memory constraints, whatever) 
the
   mbuf TX ring may not have enough slots free and would allocate slot 0. This 
is
   used as the freelist head pointer to represent "free" mbuf TX ring slots; 
setting
   this to an actual mbuf value rather than an id crashes the code.
   
   This commit introduces some basic code to track the TX mbuf ring use and then
   (hopefully!) ensures that enough slots are free in said TX mbuf ring before 
it
   enters the actual work loop.
   
   A few notes:
   
   * Similar logic needs to be introduced to check there are enough actual slots
     available in the xenbuf TX ring. There's some logic which is invoked 
earlier
     but it doesn't hard-check against the number of available ring slots.
     Its trivial to do; I'll do it in a subsequent commit.
   
   * As I've now commented in the source, it is likely possible to deadlock the
     driver under certain conditions where the rings aren't receiving any 
changes
     (which I should enumerate) and thus Xen doesn't send any further software
     interrupts. I need to make sure that the timer(s) are running right and
     the queues are periodically kicked.
   
   PR:          134926
 
 Modified:
   head/sys/dev/xen/netfront/netfront.c
 
 Modified: head/sys/dev/xen/netfront/netfront.c
 ==============================================================================
 --- head/sys/dev/xen/netfront/netfront.c       Wed May 27 01:56:37 2009        
(r192870)
 +++ head/sys/dev/xen/netfront/netfront.c       Wed May 27 02:49:08 2009        
(r192871)
 @@ -176,6 +176,7 @@ static int xennet_get_responses(struct n
   */
  struct xn_chain_data {
                struct mbuf             *xn_tx_chain[NET_TX_RING_SIZE+1];
 +              int                     xn_tx_chain_cnt;
                struct mbuf             *xn_rx_chain[NET_RX_RING_SIZE+1];
  };
  
 @@ -727,6 +728,10 @@ netif_release_tx_bufs(struct netfront_in
                    np->grant_tx_ref[i]);
                np->grant_tx_ref[i] = GRANT_INVALID_REF;
                add_id_to_freelist(np->tx_mbufs, i);
 +              np->xn_cdata.xn_tx_chain_cnt--;
 +              if (np->xn_cdata.xn_tx_chain_cnt < 0) {
 +                      panic("netif_release_tx_bufs: tx_chain_cnt must be >= 
0");
 +              }
                m_freem(m);
        }
  }
 @@ -1089,6 +1094,10 @@ xn_txeof(struct netfront_info *np)
                        
                        np->xn_cdata.xn_tx_chain[id] = NULL;
                        add_id_to_freelist(np->xn_cdata.xn_tx_chain, id);
 +                      np->xn_cdata.xn_tx_chain_cnt--;
 +                      if (np->xn_cdata.xn_tx_chain_cnt < 0) {
 +                              panic("netif_release_tx_bufs: tx_chain_cnt must 
be >= 0");
 +                      }
                        m_free(m);
                }
                np->tx.rsp_cons = prod;
 @@ -1417,7 +1426,6 @@ xn_start_locked(struct ifnet *ifp) 
                        ifp->if_drv_flags |= IFF_DRV_OACTIVE;
                        break;
                }
 -              
  
                /*
                 * Defragment the mbuf if necessary.
 @@ -1433,6 +1441,36 @@ xn_start_locked(struct ifnet *ifp) 
                        m_head = m;
                }
  
 +              /* Determine how many fragments now exist */
 +              for (m = m_head, nfrags = 0; m; m = m->m_next)
 +                      nfrags++;
 +
 +              /*
 +               * Don't attempt to queue this packet if there aren't enough 
free entries in the chain.
 +               * There isn't a 1:1 correspondance between the mbuf TX ring 
and the xenbus TX ring.
 +               * xn_txeof() may need to be called to free up some slots.
 +               *
 +               * It is quite possible that this can be later eliminated if it 
turns out that partial
 +               * packets can be pushed into the ringbuffer, with fragments 
pushed in when further slots
 +               * free up.
 +               *
 +               * It is also quite possible that the driver will lock up - Xen 
may not send another
 +               * interrupt to kick the tx/rx processing if the xenbus RX ring 
is full and xenbus TX ring
 +               * is empty - no further TX work can be done until space is 
made in the TX mbuf ring and
 +               * the RX side may be waiting for TX data to continue. It is 
quite possible some timer
 +               * event should be created to kick TX/RX processing along in 
certain conditions.
 +               */
 +
 +              /* its not +1 like the allocation because we need to keep slot 
[0] free for the freelist head */
 +              if (sc->xn_cdata.xn_tx_chain_cnt + nfrags >= NET_TX_RING_SIZE) {
 +                      printf("xn_start_locked: xn_tx_chain_cnt (%d) + nfrags 
%d >= NET_TX_RING_SIZE (%d); must be full!\n",
 +                          (int) sc->xn_cdata.xn_tx_chain_cnt, (int) nfrags, 
(int) NET_TX_RING_SIZE);
 +                      IF_PREPEND(&ifp->if_snd, m_head);
 +                      ifp->if_drv_flags |= IFF_DRV_OACTIVE;
 +                      break;
 +              }
 +
 +
                /*
                 * Start packing the mbufs in this chain into
                 * the fragment pointers. Stop when we run out
 @@ -1443,6 +1481,11 @@ xn_start_locked(struct ifnet *ifp) 
                for (m = m_head; m; m = m->m_next) {
                        tx = RING_GET_REQUEST(&sc->tx, i);
                        id = get_id_from_freelist(sc->xn_cdata.xn_tx_chain);
 +                      if (id == 0)
 +                              panic("xn_start_locked: was allocated the 
freelist head!\n");
 +                      sc->xn_cdata.xn_tx_chain_cnt++;
 +                      if (sc->xn_cdata.xn_tx_chain_cnt >= NET_TX_RING_SIZE+1)
 +                              panic("xn_start_locked: tx_chain_cnt must be < 
NET_TX_RING_SIZE+1\n");
                        sc->xn_cdata.xn_tx_chain[id] = m;
                        tx->id = id;
                        ref = gnttab_claim_grant_reference(&sc->gref_tx_head);
 _______________________________________________
 svn-src-...@freebsd.org mailing list
 http://lists.freebsd.org/mailman/listinfo/svn-src-all
 To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
 
_______________________________________________
freebsd-xen@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "freebsd-xen-unsubscr...@freebsd.org"

Reply via email to