linux-kernel - Re: [PATCH 3/5] cifsd: add file operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Mar 2021 08:15:12 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Namjae Jeon <namjae.jeon@...sung.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-cifs@...r.kernel.org,
        linux-cifsd-devel@...ts.sourceforge.net, smfrench@...il.com,
        senozhatsky@...omium.org, hyc.lee@...il.com,
        viro@...iv.linux.org.uk, hch@....de, hch@...radead.org,
        ronniesahlberg@...il.com, aurelien.aptel@...il.com,
        aaptel@...e.com, sandeen@...deen.net, dan.carpenter@...cle.com,
        colin.king@...onical.com, rdunlap@...radead.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steve French <stfrench@...rosoft.com>
Subject: Re: [PATCH 3/5] cifsd: add file operations

On Mon, Mar 22, 2021 at 02:13:42PM +0900, Namjae Jeon wrote:
> This adds file operations and buffer pool for cifsd.

Why do you want this buffer pool?  Do you not trust the
slab allocator to be able to do its job?  Because what you have
here looks slower than the slab allocator.

Let's follow this through for the best-case scenario (a buffer of the right
size already exists):

> +void *ksmbd_find_buffer(size_t size)
> +{
> +	struct wm *wm;
> +
> +	wm = find_wm(size);
> +
> +	WARN_ON(!wm);
> +	if (wm)
> +		return wm->buffer;
> +	return NULL;
> +}

OK, simple, we just call find_wm().

> +static struct wm *find_wm(size_t size)
> +{
> +	struct wm_list *wm_list;
> +	struct wm *wm;
> +
> +	wm_list = match_wm_list(size);

First we find the list for this buffer ...

> +static struct wm_list *match_wm_list(size_t size)
> +{
> +	struct wm_list *l, *rl = NULL;
> +
> +	read_lock(&wm_lists_lock);
> +	list_for_each_entry(l, &wm_lists, list) {
> +		if (l->sz == size) {
> +			rl = l;
> +			break;
> +		}
> +	}
> +	read_unlock(&wm_lists_lock);
> +	return rl;
> +}

... by taking an rwlock, and walking a linked list?!  Uh ...

> +	while (1) {
> +		spin_lock(&wm_list->wm_lock);
> +		if (!list_empty(&wm_list->idle_wm)) {
> +			wm = list_entry(wm_list->idle_wm.next,
> +					struct wm,
> +					list);
> +			list_del(&wm->list);
> +			spin_unlock(&wm_list->wm_lock);
> +			return wm;

Great!  We found one!  And all it cost us was acquiring a global rwlock,
walking a linked list to find a wmlist, then a per-wmlist spinlock.

Meanwhile, there's no guarantee the buffer we found is on the local
NUMA node.

Compare to slub, allocating from a kmem_cache (assuming you create
one for each buffer size ...):

void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
        void *ret = slab_alloc(s, gfpflags, _RET_IP_, s->object_size);

static __always_inline void *slab_alloc(struct kmem_cache *s,
                gfp_t gfpflags, unsigned long addr, size_t orig_size)
        return slab_alloc_node(s, gfpflags, NUMA_NO_NODE, addr, orig_size);

static __always_inline void *slab_alloc_node(struct kmem_cache *s,
                gfp_t gfpflags, int node, unsigned long addr, size_t orig_size)
        do {
                tid = this_cpu_read(s->cpu_slab->tid);
                c = raw_cpu_ptr(s->cpu_slab);
        } while (IS_ENABLED(CONFIG_PREEMPTION) &&
                 unlikely(tid != READ_ONCE(c->tid)));
        object = c->freelist;
        page = c->page;
        if (unlikely(!object || !page || !node_match(page, node))) {
                object = __slab_alloc(s, gfpflags, node, addr, c);
        } else {
                void *next_object = get_freepointer_safe(s, object);
                if (unlikely(!this_cpu_cmpxchg_double(
                                s->cpu_slab->freelist, s->cpu_slab->tid,
                                object, tid,
                                next_object, next_tid(tid)))) {

                        note_cmpxchg_failure("slab_alloc", s, tid);
                        goto redo;
                }
                prefetch_freepointer(s, next_object);
                stat(s, ALLOC_FASTPATH);

No lock, anywhere.  Lots of percpu goodness, so you get memory allocated
on your local node.

What's the scenario for which your allocator performs better than slub,
on a typical machine that serves enough SMB that it's worth having an
in-kernel SMBD?