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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
