Re: Fixing pool_cache_invalidate(9), final step

[Lars Heidieker <lars.heidieker%googlemail.com@localhost>]

hi folks,

On Tue, May 29, 2012 at 9:03 PM, Jean-Yves Migeon <jym%netbsd.org@localhost> 
wrote:
>
> -current: I would like to take a more radical step and perform the
> xcall(9) directly from pool_cache_invalidate(9). Given that xcall(9)
> broadcasts cannot be sent from interrupt or softint context (because of
> the use of condvars), this change is likely to expose bugs in code paths
> where invalidation is handled from interrupt context (which is a bad
> thing IMHO). See current-pcache-invalidate.diff.
>

during looking at the patch (current version one) I came across one
pitfall (if I'm not mistaken) pool_cache_invalidate_cpu will release
the per cpu pool_cache_cpu_t struct which only should happen if the
cache is about to be destroyed.
the problem lies in that pool_cache_invalidate_groups releases
pool_cache_groups during normal pool_cache_drain activity but
pool_cache_invalidate_cpu is intended for getting the pool_cache ready
for destruction.
calling pool_cache_invalidate_cpu via xcall from pool_drain =>
pool_cache_invalidate is problematic.
for the pool_cache destruction case we have to make sure that any
pool_cache_invalidate with it's pool_cache_invalidate_groug calls and
probably xcall that do the cpu local cache to pool_cache global
transfer have finished before pool_cache_invalidate_cpu is called.
if pool_cache_xcall is called from pool_drain the we could rejoin
pool_drain_start and pool_drain_end....
if my grepping is right pool_reclaim/pool_cache_reclaim are only user
in subr_pool.c (for _HARDKERNEL) so draining should indeed only happen
from the pageout daemon and not from interrupt context at all, so we
should (if I'm not mistaken) be able to simplify be integrating
pool_drain_start/end and calling the pool_cache_xcall from
pool_cache_invalidate...
currently with the split pool_drain_start/end the time the xcall runs
might on SMP be used to drain the buffer pool, the only different that
comes to my mind quickly.

> While we are at it, I would like to make the routine asynchronous for
> -current, ie. it broadcasts the invalidation but do not wait for all
> CPUs to finish executing the xcall(9). Reason behind is that each CPU is
> responsible for the invalidation of its objects, and should not try to
> feed them back to the global pool before the invalidation caller may
> finally invalidate them globally. Waste of time, especially as these
> objects can be freed safely (ctor/dtor shall implement their own
> internal locking, see pool_cache(9)).
>

we might do this async I think but we need it synced for the
pool_cache destruction case...

> For synchronous invalidation I will add pool_cache_invalidate_sync() in
> case someone needs it (I do with Xen), but I am not too fond of the
> *_sync() name. Suggestions gladly appreciated.
>
> Comments/questions on the patches are welcome.
>
> For those willing to read the initial (and a bit outdated) discussion,
> see http://mail-index.netbsd.org/current-users/2011/09/24/msg017862.html
>


kind regards,
lars


-- 
Mystische Erklärungen:
Die mystischen Erklärungen gelten für tief;
die Wahrheit ist, dass sie noch nicht einmal oberflächlich sind.
   -- Friedrich Nietzsche
   [ Die Fröhliche Wissenschaft Buch 3, 126 ]