From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754193AbbIWKdA (ORCPT ); Wed, 23 Sep 2015 06:33:00 -0400 Received: from mail-la0-f42.google.com ([209.85.215.42]:36774 "EHLO mail-la0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753450AbbIWKc5 (ORCPT ); Wed, 23 Sep 2015 06:32:57 -0400 From: Rasmus Villemoes To: Andi Kleen Cc: tytso@mit.edu, linux-kernel@vger.kernel.org, kirill.shutemov@linux.intel.com, herbert@gondor.apana.org.au, Andi Kleen Subject: Re: [PATCH 1/3] Make /dev/urandom scalable Organization: D03 References: <1442963767-14945-1-git-send-email-andi@firstfloor.org> X-Hashcash: 1:20:150923:andi@firstfloor.org::M0UIgqs3OOafZN1f:0000000000000000000000000000000000000000000x6t X-Hashcash: 1:20:150923:ak@linux.intel.com::9bhGH2cEdIsIqlbF:00000000000000000000000000000000000000000001xff X-Hashcash: 1:20:150923:tytso@mit.edu::/NSWd+Gc5oa/C6Ii:00003FbW X-Hashcash: 1:20:150923:linux-kernel@vger.kernel.org::uMGnIOzk6N032bRs:0000000000000000000000000000000003bnp X-Hashcash: 1:20:150923:herbert@gondor.apana.org.au::2YEy0OvjIjQGl5cR:00000000000000000000000000000000009M14 X-Hashcash: 1:20:150923:kirill.shutemov@linux.intel.com::R1EoeaCUzL1f5RTM:000000000000000000000000000000CSuy Date: Wed, 23 Sep 2015 12:32:53 +0200 In-Reply-To: <1442963767-14945-1-git-send-email-andi@firstfloor.org> (Andi Kleen's message of "Tue, 22 Sep 2015 16:16:05 -0700") Message-ID: <87r3lpo27e.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 23 2015, Andi Kleen wrote: > @@ -467,7 +478,7 @@ static struct entropy_store blocking_pool = { > > static struct entropy_store nonblocking_pool = { > .poolinfo = &poolinfo_table[1], > - .name = "nonblocking", > + .name = "nonblocking 0", > .pull = &input_pool, > .lock = __SPIN_LOCK_UNLOCKED(nonblocking_pool.lock), > .pool = nonblocking_pool_data, > @@ -475,6 +486,32 @@ static struct entropy_store nonblocking_pool = { > push_to_pool), > }; > > +/* > + * Per NUMA node nonblocking pool. This avoids lock contention > + * when many processes extract data from /dev/urandom in parallel. > + * /dev/random stays single instance for now. > + */ > +static struct entropy_store **nonblocking_node_pool __read_mostly; > + > +#define for_each_nb_pool(i, pool) for (i = 0; i < num_possible_nodes(); i++) { \ > + pool = nonblocking_node_pool[i]; > +#define end_for_each_nb() } You can avoid the need for end_for_each_nb (end_for_each_nb_pool?) by writing the condition i < num_possible_nodes() && (pool = nonblocking_node_pool[i], 1) [if you keep it, please be consistent in whether end_for_each_nb() is followed by ; or not, or add do{}while(0) and make the rule that it is]. But more importantly: Won't this generate a function call (ultimately to bitmap_weight) for each iteration, at least for large enough CONFIG_NODES_SHIFT? It's probably not very expensive, but would be nice to avoid. > +static inline struct entropy_store *get_nonblocking_pool(void) > +{ > + struct entropy_store *pool = &nonblocking_pool; > + > + /* > + * Non node 0 pools may take longer to initialize. Keep using > + * the boot nonblocking pool while this happens. > + */ > + if (nonblocking_node_pool) > + pool = nonblocking_node_pool[numa_node_id()]; > + if (!pool->initialized) > + pool = &nonblocking_pool; > + return pool; > +} I assume this can't get called concurrently with rand_initialize (otherwise pool may be NULL even if nonblocking_node_pool is non-NULL). > > @@ -1393,9 +1446,32 @@ static void init_std_data(struct entropy_store *r) > */ > static int rand_initialize(void) > { > + int i; > + int num_nodes = num_possible_nodes(); > + char name[40]; > + > + nonblocking_node_pool = kzalloc(num_nodes * sizeof(void *), > + GFP_KERNEL|__GFP_NOFAIL); > + Why kzalloc, when you immediately initialize all elements? New uses of __GFP_NOFAIL seem to be frowned upon. How hard would it be to just fall back to only using the single statically allocated pool? Does rand_initialize get called before or after other initialization code updates node_possible_map to reflect the actual possible number of nodes? If before, won't we be wasting a lot of memory (not to mention that we then might as well allocate all the nonblocking pools statically based on MAX_NUMNODES). > init_std_data(&input_pool); > init_std_data(&blocking_pool); > + nonblocking_node_pool[0] = &nonblocking_pool; > init_std_data(&nonblocking_pool); > + for (i = 1; i < num_nodes; i++) { > + struct entropy_store *pool = kzalloc(sizeof(struct entropy_store), > + GFP_KERNEL|__GFP_NOFAIL); > + nonblocking_node_pool[i] = pool; > + pool->poolinfo = &poolinfo_table[1]; > + pool->pull = &input_pool; > + spin_lock_init(&pool->lock); > + /* pool data not cleared intentionally */ > + pool->pool = kmalloc(sizeof(nonblocking_pool_data), > + GFP_KERNEL|__GFP_NOFAIL); > + INIT_WORK(&pool->push_work, push_to_pool); > + snprintf(name, sizeof name, "nonblocking pool %d", i); > + pool->name = kstrdup(name, GFP_KERNEL|__GFP_NOFAIL); kasprintf(). Also, you renamed the static pool to "nonblocking 0". Rasmus