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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>