lkml.org 
[lkml]   [2012]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] zcache: don't limit number of pools per client
From
Date
On Tue, 2012-06-05 at 15:19 -0500, Seth Jennings wrote:
> > + pool = idr_find(&cli->tmem_pools, poolid);
> > + if (pool != NULL)
> > + atomic_inc(&pool->refcount);
>
>
> This is called on the main path, so it needs to be fast. There is so much
> contention elsewhere in the stack I don't think it'll be an issue. It looks
> like idr_find() is fast, even though it contains a loop. Just needs to be
> considered.

Agreed. idr actually uses something which looks like a hash table, so it
should be fast enough for this case.

> > +retry:
> > + r = idr_pre_get(&cli->tmem_pools, GFP_ATOMIC);
>
> > + if (r != 1) {
> > + kfree(pool);
> > + pr_info("zcache: pool creation failed: out of memory\n");
> > + goto out;
> > + }
> > + r = idr_get_new(&cli->tmem_pools, pool, &poolid);
> > + switch (r) {
> > + case 0:
> > + break;
> > + case -EAGAIN:
> > + goto retry;
> > + default:
> > + pr_info("zcache: pool creation failed: error %d\n", r);
> > kfree(pool);
> > - poolid = -1;
> > goto out;
> > }
> > +
>
>
> how about:
> =====
> do {
> r = idr_pre_get(&cli->tmem_pools, GFP_ATOMIC);
> if (r != 1) {
> kfree(pool);
> pr_info("zcache: pool creation failed: out of memory\n");
> goto out;
> }
> r = idr_get_new(&cli->tmem_pools, pool, &poolid);
> }
> while (r == -EAGAIN)
>
> if (r) {
> pr_info("zcache: pool creation failed: error %d\n", r);
> kfree(pool);
> goto out;
> }
> =====
> so we can lose the label/goto.
>
> Also, do we want GFP_ATOMIC? Why not GFP_KERNEL?

I wasn't sure about the context this code runs at, and assumed it's
atomic one due to the kmalloc allocating with GFP_ATOMIC just couple of
lines above the idr_pre_get. If that's wrong, we can switch that kmalloc
as well.



\
 
 \ /
  Last update: 2012-06-05 23:01    [W:0.031 / U:1.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site