Re: workqueue in if_bnx: static struct work * seems wrong

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

On Thursday 01 March 2012 09:29:32 Manuel Bouyer wrote:
> On Thu, Mar 01, 2012 at 09:23:34AM -0700, Sverre Froyen wrote:
> > On Monday 27 February 2012 02:22:20 Jean-Yves Migeon wrote:
> > > On Sun, 26 Feb 2012 22:05:59 +0100, Manuel Bouyer wrote:
> > > > On Sun, Feb 26, 2012 at 05:22:50PM +0100, Jean-Yves Migeon wrote:
> > > >[...]
> > > 
> > > Yep, you are right. I just had a look at workqueue(9) implementation
> > > this morning, and multiple bnx(4) instances would badly break with
> > > anything besides SIMPLEQ.
> > > 
> > > Having a wk allocated in sc is the way to go.
> > 
> > Instead of every driver implementing its own memory management, would it
> > not make more sense to use an common mechanism like, e.g., pool_cache?
> 
> A pool for a single per-instance structure which is completely static
> deosn't make sense. Just declare it static in the softc ...

Sorry, I was following up to the latest message on the thread and doing a poor 
job of quoting.

I was really thinking about the change that prompted this discussion (if_bnx.c 
rev 1.44). In particular the replacement of

        bnx_alloc_pkts(sc)

with

        workqueue_enqueue(sc->bnx_wq, &bnx_wk, NULL);
        SET(sc->bnx_flags, BNX_ALLOC_PKTS_FLAG);

which was made in order to avoid memory allocation in interrupt context.

It seems to me that this could be handled by the pool code instead. Perhaps as 
follows:

1) Create pool_caches of header structures (one per buffer size).

2) Use a custom allocator that does the bus_dma dance and attaches the 
resulting memory pointers to the header structure.

3) Require a minimum number of free items (pool_cache_setlowat).

4) Call pool_cache_get in the driver to obtain memory buffers.

I attempted to get this working in the iwn driver( for the rx buffers) but ran 
into some issues in the pool code that prevented a simple minded version from 
working. I believe the problems can be fixed, however.

Regards,
Sverre