Re: rbuf starvation in the iwn driver

[Sverre Froyen <sverre%viewmark.com@localhost>]

On Wed April 7 2010 12:40:16 David Young wrote:
> On Wed, Apr 07, 2010 at 11:30:24AM -0600, Sverre Froyen wrote:
> > On Tue April 6 2010 06:26:10 Manuel Bouyer wrote:
> > ...
> >
> > > What is doing this driver looks somewhat wrong to me.
> > > It can use its rbufs as external mbuf storage, and pass this to the
> > > network stack, as long as there's enough free rbufs. When the number of
> > > free rbufs is below the limit, it should stop giving them to the
> > > network stack, and copy the received data to a ordinary mbuf cluster
> > > instead. See for example if_nfe.c:
> >
> > I've been looking at the iwn_rx_done code to see how I can fix this. One
> > option is to use the code from iwn_rx_intr in if_iwn.c rev 1.32 (I
> > presume I can figure out the needed bus_dmamap changes by looking at
> > if_nfe.c).
> >
> > However, it looks like the current OpenBSD and FreeBSD drivers do not use
> > private storage. FreeBSD just gets an mbuf cluster from m_getjcl and
> > OpenBSD uses MCLGETI. Is this an option for NetBSD? Why was private
> > storage used in the first place? For speed reasons? Damien Bergamini
> > warned me there would likely be alignment issues.
> 
> Based a cursory scan of the code, iwn(4) provides a 4kB,
> physically-contiguous Rx buffer to the firmware, i.e., a buffer large
> enough to fit an 802.11 aggregate packet (A-MPDU).  Don't OpenBSD and
> FreeBSD provide the firmware with 4kB Rx buffers, too?  On most (all?)
> NetBSD architectures, an mbuf cluster is just 2kB, so MCLGET() won't do.
> I wouldn't trust MEXTMALLOC() to return a physically-contiguous region.

#define MEXTMALLOC(m, size, how) 
do {
        (m)->m_ext_storage.ext_buf = 
            (void *)malloc((size), mbtypes[(m)->m_type], (how)); 
...

with a single malloc looks like it would be contiguous but probably slow. It 
should also be properly aligned according to the malloc(9) man page.

FreeBSD and OpenBSD are both using several pool_cache(9)s (or the equivalent) 
of predefined (useful) sizes.  Perhaps MEXTMALLOC could be extended to use a 
pool_cache if the correct size pool_cache is available, falling back on malloc 
if not, or if the call to pool_cache_get_paddr failed (can it fail?).

Sverre