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

[Dan Streetman <ddstreet@ieee.org>]

On Sun, Aug 9, 2015 at 8:49 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> 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 ;-)

Ah, well sure that looks better, I'll send a patch (or roll it into a
patch set resend).

Thanks!

>
>
> [..]
>> > 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