Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751967AbbHNUDL (ORCPT ); Fri, 14 Aug 2015 16:03:11 -0400 Received: from mail-ig0-f171.google.com ([209.85.213.171]:36931 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437AbbHNUDJ (ORCPT ); Fri, 14 Aug 2015 16:03:09 -0400 MIME-Version: 1.0 In-Reply-To: <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> <20150810004912.GB645@swordfish> From: Dan Streetman Date: Fri, 14 Aug 2015 16:02:29 -0400 X-Google-Sender-Auth: LNWFD5PJYrqb6BkZrpKhtII8Z-s Message-ID: Subject: Re: [PATCH 2/3] zswap: dynamic pool creation To: Sergey Senozhatsky Cc: Seth Jennings , Andrew Morton , Linux-MM , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3113 Lines: 91 On Sun, Aug 9, 2015 at 8:49 PM, Sergey Senozhatsky 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 -- 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/