Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752198AbbHJAsj (ORCPT ); Sun, 9 Aug 2015 20:48:39 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:35261 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677AbbHJAsg (ORCPT ); Sun, 9 Aug 2015 20:48:36 -0400 Date: Mon, 10 Aug 2015 09:49:12 +0900 From: Sergey Senozhatsky To: Dan Streetman Cc: Sergey Senozhatsky , Seth Jennings , Andrew Morton , Linux-MM , linux-kernel Subject: Re: [PATCH 2/3] zswap: dynamic pool creation Message-ID: <20150810004912.GB645@swordfish> References: <1438782403-29496-1-git-send-email-ddstreet@ieee.org> <1438782403-29496-3-git-send-email-ddstreet@ieee.org> <20150807063056.GG1891@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2811 Lines: 83 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/