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

[Dan Streetman]

On Fri, Aug 7, 2015 at 10:24 AM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
> 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.

i knew there was a reason i added the tfm_name ;-)

since ->tfm is a percpu, we add a notifier for added/removed cpus.
when a cpu is added, we create a new tfm for it.  If we don't have the
tfm_name separate from the percpu ->tfm, we have to check some other
cpu's tfm for its name, and i don't think the complexity of checking
what cpus are present *and* have a ->tfm allocated already just to get
the name is worth it, for only 64 bytes ;-)

>
>>
>>> +     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/