Re: CVS commit: src/sys/dev/raidframe

[Greg Oster <oster%cs.usask.ca@localhost>]

Bill Studenmund writes:
> 
> 
> On Fri, Jan 23, 2004 at 01:57:08AM +0000, Greg Oster wrote:
> >=20
> > Module Name:        src
> > Committed By:       oster
> > Date:               Fri Jan 23 01:57:08 UTC 2004
> >=20
> > Modified Files:
> >     src/sys/dev/raidframe: rf_stripelocks.c
> >=20
> > Log Message:
> > Always ask for a new RF_StripeLockDesc_t "just in case", and then
> > give it back if we don't need it.  If we don't allocate it before
> > we take our lock, LOCKDEBUG (rightfully) complains that we're trying
> > to grab something from the pool with PR_WAITOK.  This code (and the
> > PR_WAITOK in particular) really needs to be revisited at some point.
> 
> How often do we go through this path?

"lots".  At least once per stripe access.

> Would it make sense to have a global single cache? Have a simplelock and
> a global RF_StripeLockDesc_t *. On entry, if there's one in the cache, 
> take it. Otherwise do the allocation you have now. Do what you do now. 
> Then grab the cache lock. If there's not one in the cache, put this one 
> in. Otherwise, free it.

Hmmm...  That might work quite well... both here, and in a number of 
other places that will need the same "fix".  The old code used a 
"freelist", and it might be worth-while to return to that sort of a
"cache" structure for these.. (perhaps still fed by pools.. I'm not sure..)
 
> I think the cache lock twiddling and comparison against NULL would be less
> work than the memory allocation. :-)

The "allocation" right now is just grabbing one from the pool, so hopefully 
that's not *too* expensive.  But what you suggest should improve things
over just simple pool allocations, I think.
 
> This kind of change would trade off usually having one RF_StripeLockDesc_t
> lying around for not having to allocate then deallocate it each pass.

Yes.. but that's not too big of a deal, given the frequency with 
which they are used...  (The pool starts with 32 of them, IIRC, and 
the high-water is set to 128).
 
> With __predict_true() and __predict_false() (are those the names?), the
> code can be efficient in the common case.
> 
> You're the only one, though, who knows if it'd be worth it. :-)

I think it would :)  There are a lot of memory allocation bits that 
need to be (re-)evaluated.  This just happened to be one that was 
'nearby'.

Thanks for the suggestion.

Later...

Greg Oster