linux-kernel - Re: [PATCH v3 1/3] lib: find_*

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150208184823.31349.qmail@ns.horizon.com>
Date:	8 Feb 2015 13:48:23 -0500
From:	"George Spelvin" <linux@...izon.com>
To:	akpm@...ux-foundation.org, chris@...is-wilson.co.uk,
	davem@...emloft.net, dborkman@...hat.com,
	hannes@...essinduktion.org, klimov.linux@...il.com,
	laijs@...fujitsu.com, linux@...izon.com, msalter@...hat.com,
	takahiro.akashi@...aro.org, tgraf@...g.ch,
	valentinrothberg@...il.com, yury.norov@...il.com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] lib: find_*_bit reimplementation

This basically has my Reviewed-by: (I'll send it in a few hours
when I have time to do a real final copy-editing), but a few minor
notes:

>+		/*
>+		 * This is an equvalent for:
>+		 *
>+		 * tmp = find_set ? addr[start / BITS_PER_LONG]
>+		 *	: ~addr[start / BITS_PER_LONG];
>+		 *
>+		 * but saves a branch condition.
>+		 *
>+		 * Thanks George Spelvin <linux@...izon.com> for idea.
>+		 */
>+		tmp = addr[start / BITS_PER_LONG] ^ mask;

1. There's no need for detailed credit for such a trivial and obvious
   thing.  If you want to comment it, describe the use of the parameter
   in the function header, e.g.

+/*
+ * "mask" is either 0 or ~0UL and XORed into each fetched word, to select between
+ * searching for one bits and zero bits.  If this function is inlined, GCC is
+ * smart enough to propagate the constant.
+ */

2. The variable might be more meaningfully named "xor" or "invert"; "mask"
   suggests something that is &-ed with a value.


> unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> 				 unsigned long offset)
> {
>+	return _find_next_bit(addr, size, offset, ~0UL);	<---
> }

> unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
> {
>+	unsigned long idx;
> 
>+	for (idx = 0; idx * BITS_PER_LONG < size; idx++) {
>+		if (addr[idx] != ULONG_MAX)			<---
>+			return min(idx * BITS_PER_LONG + ffz(addr[idx]), size);
> 	}
> 
>+	return size;
> }

3. Using two names (ULONG_MAX and ~0UL) for the same thing is a bit odd;
   you might want to be consistent.

I'll ack it either way; none of these are significant technical issues.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/