On Mon, Jul 30, 2018 at 11:34:23AM +0200, Dominique Martinet wrote:
> -static int p9_fcall_alloc(struct p9_fcall *fc, int alloc_msize)
> +static int p9_fcall_alloc(struct p9_client *c, struct p9_fcall *fc,
> + int alloc_msize)
> {
> - fc->sdata = kmalloc(alloc_msize, GFP_NOFS);
> + if (c->fcall_cache && alloc_msize == c->msize)
> + fc->sdata = kmem_cache_alloc(c->fcall_cache, GFP_NOFS);
> + else
> + fc->sdata = kmalloc(alloc_msize, GFP_NOFS);
Could you simplify this by initialising c->msize to 0 and then this
can simply be:
> + if (alloc_msize == c->msize)
...
> +void p9_fcall_free(struct p9_client *c, struct p9_fcall *fc)
> +{
> + /* sdata can be NULL for interrupted requests in trans_rdma,
> + * and kmem_cache_free does not do NULL-check for us
> + */
> + if (unlikely(!fc->sdata))
> + return;
> +
> + if (c->fcall_cache && fc->capacity == c->msize)
> + kmem_cache_free(c->fcall_cache, fc->sdata);
> + else
> + kfree(fc->sdata);
> +}
Is it possible for fcall_cache to be allocated before fcall_free is
called? I'm concerned we might do this:
allocate message A
allocate message B
receive response A
allocate fcall_cache
receive response B
and then we'd call kmem_cache_free() for something allocated by kmalloc(),
which works with slab and slub, but doesn't work with slob (alas).
> @@ -980,6 +1000,9 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
> if (err)
> goto close_trans;
>
> + clnt->fcall_cache = kmem_cache_create("9p-fcall-cache", clnt->msize,
> + 0, 0, NULL);
> +
If we have slab merging turned off, or we have two mounts from servers
with different msizes, we'll end up with two slabs called 9p-fcall-cache.
I'm OK with that, but are you?
