[3.13 PATCH 0/2] random: bytes vs. bits

[3.13 PATCH 0/2] random: bytes vs. bits

[Greg Price]

The new when-to-push logic in v3.13-rc1~61^2~6 "random: push extra
entropy to the output pools" uses the name "entropy_bytes" for a
quantity that's actually in bits.  This results in confusing,
buggy-looking lines both where the variable is set and where it's
used.  Fortunately it's all consistent if the variable is understood
as counting bits, so the behavior is fine.

This code is new in v3.13-rc1, so fixing the name seems in order for
3.13.  The first patch here just does that.

One cause of the mistake is probably that the names of
random_read_wakeup_thresh and random_write_wakeup_thresh don't
indicate what units they're in.  This is an especially acute issue now
that we have three units for entropy in different places -- bytes,
bits, and 1 / (1 << ENTROPY_SHIFT) fractions of a bit.  The second
patch clarifies the units in both names.  (Maybe other names should be
clarified too.)  This may as well be post-3.13.

Cheers,
Greg


Greg Price (2):
  random: entropy_bytes is actually bits
  random: clarify bits/bytes in wakeup thresholds

 drivers/char/random.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
1.8.3.2

[3.13 PATCH 1/2] random: entropy_bytes is actually bits

[Greg Price]

The variable 'entropy_bytes' is set from an expression that actually
counts bits.  Fortunately it's also only compared to values that also
count bits.  Rename it accordingly.

Signed-off-by: Greg Price <price@mit.edu>
---
 drivers/char/random.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 429b75bb6..dc9b9af20 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -666,10 +666,10 @@ retry:
 				  r->entropy_total, _RET_IP_);
 
 	if (r == &input_pool) {
-		int entropy_bytes = entropy_count >> ENTROPY_SHIFT;
+		int entropy_bits = entropy_count >> ENTROPY_SHIFT;
 
 		/* should we wake readers? */
-		if (entropy_bytes >= random_read_wakeup_thresh) {
+		if (entropy_bits >= random_read_wakeup_thresh) {
 			wake_up_interruptible(&random_read_wait);
 			kill_fasync(&fasync, SIGIO, POLL_IN);
 		}
@@ -678,7 +678,7 @@ retry:
 		 * forth between them, until the output pools are 75%
 		 * full.
 		 */
-		if (entropy_bytes > random_write_wakeup_thresh &&
+		if (entropy_bits > random_write_wakeup_thresh &&
 		    r->initialized &&
 		    r->entropy_total >= 2*random_read_wakeup_thresh) {
 			static struct entropy_store *last = &blocking_pool;
-- 
1.8.3.2

[PATCH 2/2] random: clarify bits/bytes in wakeup thresholds

[Greg Price]

These are a recurring cause of confusion, so rename them to
hopefully be clearer.

Signed-off-by: Greg Price <price@mit.edu>
---
 drivers/char/random.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index dc9b9af20..3f87a7e88 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -295,14 +295,14 @@
  * The minimum number of bits of entropy before we wake up a read on
  * /dev/random.  Should be enough to do a significant reseed.
  */
-static int random_read_wakeup_thresh = 64;
+static int random_read_wakeup_bits = 64;
 
 /*
  * If the entropy count falls under this number of bits, then we
  * should wake up processes which are selecting or polling on write
  * access to /dev/random.
  */
-static int random_write_wakeup_thresh = 28 * OUTPUT_POOL_WORDS;
+static int random_write_wakeup_bits = 28 * OUTPUT_POOL_WORDS;
 
 /*
  * The minimum number of seconds between urandom pool resending.  We
@@ -669,7 +669,7 @@ retry:
 		int entropy_bits = entropy_count >> ENTROPY_SHIFT;
 
 		/* should we wake readers? */
-		if (entropy_bits >= random_read_wakeup_thresh) {
+		if (entropy_bits >= random_read_wakeup_bits) {
 			wake_up_interruptible(&random_read_wait);
 			kill_fasync(&fasync, SIGIO, POLL_IN);
 		}
@@ -678,9 +678,9 @@ retry:
 		 * forth between them, until the output pools are 75%
 		 * full.
 		 */
-		if (entropy_bits > random_write_wakeup_thresh &&
+		if (entropy_bits > random_write_wakeup_bits &&
 		    r->initialized &&
-		    r->entropy_total >= 2*random_read_wakeup_thresh) {
+		    r->entropy_total >= 2*random_read_wakeup_bits) {
 			static struct entropy_store *last = &blocking_pool;
 			struct entropy_store *other = &blocking_pool;
 
@@ -924,19 +924,19 @@ static void _xfer_secondary_pool(struct entropy_store *r, size_t nbytes)
 {
 	__u32	tmp[OUTPUT_POOL_WORDS];
 
-	/* For /dev/random's pool, always leave two wakeup worth's BITS */
-	int rsvd = r->limit ? 0 : random_read_wakeup_thresh/4;
+	/* For /dev/random's pool, always leave two wakeups' worth */
+	int rsvd_bytes = r->limit ? 0 : random_read_wakeup_bits / 4;
 	int bytes = nbytes;
 
-	/* pull at least as many as BYTES as wakeup BITS */
-	bytes = max_t(int, bytes, random_read_wakeup_thresh / 8);
+	/* pull at least as much as a wakeup */
+	bytes = max_t(int, bytes, random_read_wakeup_bits / 8);
 	/* but never more than the buffer size */
 	bytes = min_t(int, bytes, sizeof(tmp));
 
 	trace_xfer_secondary_pool(r->name, bytes * 8, nbytes * 8,
 				  ENTROPY_BITS(r), ENTROPY_BITS(r->pull));
 	bytes = extract_entropy(r->pull, tmp, bytes,
-				random_read_wakeup_thresh / 8, rsvd);
+				random_read_wakeup_bits / 8, rsvd_bytes);
 	mix_pool_bytes(r, tmp, bytes, NULL);
 	credit_entropy_bits(r, bytes*8);
 }
@@ -952,7 +952,7 @@ static void push_to_pool(struct work_struct *work)
 	struct entropy_store *r = container_of(work, struct entropy_store,
 					      push_work);
 	BUG_ON(!r);
-	_xfer_secondary_pool(r, random_read_wakeup_thresh/8);
+	_xfer_secondary_pool(r, random_read_wakeup_bits/8);
 	trace_push_to_pool(r->name, r->entropy_count >> ENTROPY_SHIFT,
 			   r->pull->entropy_count >> ENTROPY_SHIFT);
 }
@@ -1004,7 +1004,7 @@ retry:
 			goto retry;
 
 		if ((r->entropy_count >> ENTROPY_SHIFT)
-		    < random_write_wakeup_thresh)
+		    < random_write_wakeup_bits)
 			wakeup_write = 1;
 	}
 	spin_unlock_irqrestore(&r->lock, flags);
@@ -1313,7 +1313,7 @@ random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 
 			wait_event_interruptible(random_read_wait,
 				ENTROPY_BITS(&input_pool) >=
-				random_read_wakeup_thresh);
+				random_read_wakeup_bits);
 
 			if (signal_pending(current)) {
 				retval = -ERESTARTSYS;
@@ -1358,9 +1358,9 @@ random_poll(struct file *file, poll_table * wait)
 	poll_wait(file, &random_read_wait, wait);
 	poll_wait(file, &random_write_wait, wait);
 	mask = 0;
-	if (ENTROPY_BITS(&input_pool) >= random_read_wakeup_thresh)
+	if (ENTROPY_BITS(&input_pool) >= random_read_wakeup_bits)
 		mask |= POLLIN | POLLRDNORM;
-	if (ENTROPY_BITS(&input_pool) < random_write_wakeup_thresh)
+	if (ENTROPY_BITS(&input_pool) < random_write_wakeup_bits)
 		mask |= POLLOUT | POLLWRNORM;
 	return mask;
 }
@@ -1583,7 +1583,7 @@ struct ctl_table random_table[] = {
 	},
 	{
 		.procname	= "read_wakeup_threshold",
-		.data		= &random_read_wakeup_thresh,
+		.data		= &random_read_wakeup_bits,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
@@ -1592,7 +1592,7 @@ struct ctl_table random_table[] = {
 	},
 	{
 		.procname	= "write_wakeup_threshold",
-		.data		= &random_write_wakeup_thresh,
+		.data		= &random_write_wakeup_bits,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-- 
1.8.3.2

Re: [3.13 PATCH 1/2] random: entropy_bytes is actually bits

[Theodore Ts'o]

On Fri, Dec 06, 2013 at 09:27:52PM -0500, Greg Price wrote:
> The variable 'entropy_bytes' is set from an expression that actually
> counts bits.  Fortunately it's also only compared to values that also
> count bits.  Rename it accordingly.
> 
> Signed-off-by: Greg Price <price@mit.edu>

Thanks, applied.

					- Ted

Re: [PATCH 2/2] random: clarify bits/bytes in wakeup thresholds

[Theodore Ts'o]

On Fri, Dec 06, 2013 at 09:28:03PM -0500, Greg Price wrote:
> These are a recurring cause of confusion, so rename them to
> hopefully be clearer.
> 
> Signed-off-by: Greg Price <price@mit.edu>

Thanks, applied with some merge fix ups to get the patch to apply.

		     	  	    	- Ted