linux-kernel - Re: [PATCH 2/3] zswap: dynamic pool creation

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]

Message-ID: <20150810004912.GB645@swordfish>
Date:	Mon, 10 Aug 2015 09:49:12 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Dan Streetman <ddstreet@...e.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Seth Jennings <sjennings@...iantweb.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux-MM <linux-mm@...ck.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] zswap: dynamic pool creation

Hello,

On (08/07/15 10:24), Dan Streetman wrote:
> > On (08/05/15 09:46), Dan Streetman wrote:
> > [..]
> >> -enum comp_op {
> >> -     ZSWAP_COMPOP_COMPRESS,
> >> -     ZSWAP_COMPOP_DECOMPRESS
> >> +struct zswap_pool {
> >> +     struct zpool *zpool;
> >> +     struct kref kref;
> >> +     struct list_head list;
> >> +     struct rcu_head rcu_head;
> >> +     struct notifier_block notifier;
> >> +     char tfm_name[CRYPTO_MAX_ALG_NAME];
> >
> > do you need to keep a second CRYPTO_MAX_ALG_NAME copy? shouldn't it
> > be `tfm->__crt_alg->cra_name`, which is what
> >         crypto_tfm_alg_name(struct crypto_tfm *tfm)
> > does?
> 
> well, we don't absolutely have to keep a copy of tfm_name.  However,
> ->tfm is a __percpu variable, so each time we want to check the pool's
> tfm name, we would need to do:
> crypto_comp_name(this_cpu_ptr(pool->tfm))
> 
> nothing wrong with that really, just adds a bit more code each time we
> want to check the tfm name.  I'll send a patch to change it.
> 
> >
> >> +     struct crypto_comp * __percpu *tfm;
> >>  };
> >
> > ->tfm will be access pretty often, right? did you intentionally put it
> > at the bottom offset of `struct zswap_pool'?
> 
> no it wasn't intentional; does moving it up provide a benefit?

well, I just prefer to keep 'read mostly' pointers together. all
those cache lines, etc.

gcc 5.1, x86_64

 struct zswap_pool {
        struct zpool *zpool;
+       struct crypto_comp * __percpu *tfm;
        struct kref kref;
        struct list_head list;
        struct rcu_head rcu_head;
        struct notifier_block notifier;
        char tfm_name[CRYPTO_MAX_ALG_NAME];
-       struct crypto_comp * __percpu *tfm;
 };

../scripts/bloat-o-meter zswap.o.old zswap.o
add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-27 (-27)
function                                     old     new   delta
zswap_writeback_entry                        659     656      -3
zswap_frontswap_store                       1445    1442      -3
zswap_frontswap_load                         417     414      -3
zswap_pool_create                            438     432      -6
__zswap_cpu_comp_notifier.part               152     146      -6
__zswap_cpu_comp_notifier                    122     116      -6


you know it better ;-)


[..]
> > this one seems to be used only once. do you want to replace
> > that single usage (well, if it's really needed)
> 
> it's actually used twice, in __zswap_pool_empty() and
> __zswap_param_set().  The next patch adds __zswap_param_set().

Aha, sorry, didn't read the next patch in advance.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/