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

[Dan Streetman]

On Fri, Aug 7, 2015 at 2:30 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> wrote:
> Hello,
>
> 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?

>
> [..]
>> +static struct zswap_pool *__zswap_pool_current(void)
>>  {
>> -     return totalram_pages * zswap_max_pool_percent / 100 <
>> -             DIV_ROUND_UP(zswap_pool_total_size, PAGE_SIZE);
>> +     struct zswap_pool *pool;
>> +
>> +     pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
>> +     WARN_ON(!pool);
>> +
>> +     return pool;
>> +}
>> +
>> +static struct zswap_pool *zswap_pool_current(void)
>> +{
>> +     assert_spin_locked(&zswap_pools_lock);
>> +
>> +     return __zswap_pool_current();
>> +}
>
> 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().

>
>         WARN_ON(pool == zswap_pool_current());
> with
>         WARN_ON(pool == __zswap_pool_current);
>
> ?
>
> you can then drop zswap_pool_current()... and probably rename
> __zswap_pool_current() to zswap_pool_current().
>
>         -ss
>
>> +static struct zswap_pool *zswap_pool_current_get(void)
>> +{
>> +     struct zswap_pool *pool;
>> +
>> +     rcu_read_lock();
>> +
>> +     pool = __zswap_pool_current();
>> +     if (!pool || !zswap_pool_get(pool))
>> +             pool = NULL;
>> +
>> +     rcu_read_unlock();
>> +
>> +     return pool;
>> +}
>> +
>> +static struct zswap_pool *zswap_pool_last_get(void)
>> +{
>> +     struct zswap_pool *pool, *last = NULL;
>> +
>> +     rcu_read_lock();
>> +
>> +     list_for_each_entry_rcu(pool, &zswap_pools, list)
>> +             last = pool;
>> +     if (!WARN_ON(!last) && !zswap_pool_get(last))
>> +             last = NULL;
>> +
>> +     rcu_read_unlock();
>> +
>> +     return last;
>> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/