pool_cache_prime and ctors

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

tl;dr: Should pool_cache_prime be changed to call the ctor, enabling
drivers to use it to preallocate resources like bus_dma in a ctor?


A pool cache with a constructor manages:

- a backing store of pages full of object storage
- cheap per-CPU caches of _constructed_ objects

When you call pool_cache_get:

(a) If there is a constructed object available, it will return that.

(b) Otherwise, if there are any backing pages with free space left, it
    falls back to getting space for an object out of the backing page,
    and constructs it by calling the constructor.

(c) Otherwise, it allocates a backing page (may sleep), gets space for
    an object out of the backing page, and constructs it by calling
    the constructor.

If you prime a pool cache for n objects with pool_cache_prime(pc, n),
it ensures you can call pool_cache_get n times without hitting (c), so
you won't sleep or fail _allocating a backing page_.

But what about the constructor?  Constructors sometimes do exciting
things that may sleep for allocation like bus_dmamap_create/load and
bus_dmamem_alloc/map.  Drivers could use pool caches to share the cost
of those allocations across many object reuses, such as rx buffers.
However, bus_dmamem_alloc is absolutely forbidden in softint context
(even with BUS_DMA_NOWAIT).  So it's not enough to preallocate the
buffers up front with pool_cache_prime (even if they prevent
additional backing page allocation with pool_cache_sethardlimit).
Even if bus_dmamem_alloc were allowed with BUS_DMA_NOWAIT, it's not
great to be rummaging through the uvm page lists in softint context.

I think pool_cache_prime should ensure you can call pool_cache_get n
times without hitting (c) _or_ (b), so you won't sleep or fail
allocating a backing page _or_ constructing an object.

Thoughts?


How would this affect existing callers?  We currently have very few
uses of pool_cache_prime, and from what I can tell, they either aren't
affected or probably already assume the proposed semantics and might
actually be broken without it:

1. arm32 pmap:
   https://nxr.netbsd.org/xref/src/sys/arch/arm/arm32/pmap.c?r=1.450#1282

   Here the pool cache has a constructor but it only zeroes the object
   flushes some CPU data cache entries -- I doubt that is likely to
   cause any trouble if done at the time of pool_cache_prime (maybe it
   would be better to issue a single dsb after pool_cache_prime
   instead of one per object but I doubt it's a big deal):

   https://nxr.netbsd.org/xref/src/sys/arch/arm/arm32/pmap.c?r=1.450#6547
   https://nxr.netbsd.org/xref/src/sys/arch/arm/arm32/pmap.c?r=1.450#1653
   https://nxr.netbsd.org/xref/src/sys/arch/arm/include/arm32/pmap.h?r=1.181#578

2. sun8icrypto(4):
   https://nxr.netbsd.org/xref/src/sys/arch/arm/sunxi/sun8i_crypto.c?r=1.32#406

   I wrote this driver under the assumption that pool_cache_prime
   would call the constructors, exactly for the purpose described
   above.  I wouldn't be surprised if it simply doesn't work for IPsec
   in the kernel and crashes with KASSERT(!cpu_softintr_p()) inside
   bus_dmamem_alloc.

   https://nxr.netbsd.org/xref/src/sys/arch/arm/sunxi/sun8i_crypto.c?r=1.32#392
   https://nxr.netbsd.org/xref/src/sys/arch/arm/sunxi/sun8i_crypto.c?r=1.32#506

3. bwfm(4):
   https://nxr.netbsd.org/xref/src/sys/dev/ic/bwfm.c?r=1.37#343

   No constructor, so my proposed change can't make a difference.

   (Also I think this should not use a pool_cache -- if I understand
   correctly, for each type of task, it only makes sense for one
   instance of it to be pending at any given time, and it should drop
   the _older_ instance of each task rather than the _newer_ one.  So
   it should just have a bit map of pending tasks and a statically
   allocated argument space for each one, like wg(4) does.)

4. bwi(4):
   https://nxr.netbsd.org/xref/src/sys/dev/ic/bwi.c?r=1.45#898

   No constructor, so my propposed change can't make a difference.

   (Likewise I think this shouldn't be using a pool_cache.)

5. hifn(4):
   https://nxr.netbsd.org/xref/src/sys/dev/pci/hifn7751.c?r=1.82#405

   Same deal as sun8icrypto(4) -- constructor allocates DMA maps and
   buffers with bus_dma, and I wouldn't be surprised if this just
   doesn't work when you try to use hifn(4) for IPsec in the kernel.

   https://nxr.netbsd.org/xref/src/sys/dev/pci/hifn7751.c?r=1.82#141


Here are a few examples of drivers that use a custom lock and list of
preallocated objects instead of a pool cache to manage rx buffers:

dge(4): https://nxr.netbsd.org/xref/src/sys/dev/pci/if_dge.c?r=1.65#475
eqos(4): https://nxr.netbsd.org/xref/src/sys/dev/ic/dwc_eqos.c?r=1.56#1635
iwn(4): https://nxr.netbsd.org/xref/src/sys/dev/pci/if_iwn.c?r=1.101#6594
msk(4): https://nxr.netbsd.org/xref/src/sys/dev/pci/if_msk.c?r=1.119#638

Note that dge(4) is actually broken on MP systems because dge_freebuf
can be called (by m_ext_free) without the kernel lock held, so access
the list of rx buffers is unlocked.  Oops!  Using a pool cache would
save the trouble of adding per-driver synchronization logic like this.