linux-kernel - Re: [PATCH v2 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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 05 Feb 2015 02:45:18 +0300
From:	Yury <yury.norov@...il.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>,
	George Spelvin <linux@...izon.com>
CC:	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-kernel@...r.kernel.org,
	msalter@...hat.com, takahiro.akashi@...aro.org, tgraf@...g.ch,
	valentinrothberg@...il.com, Yury Norov <y.norov@...sung.com>
Subject: Re: [PATCH v2 1/3] lib: find_*_bit reimplementation


On 02.02.2015 15:56, Rasmus Villemoes wrote:
> On Mon, Feb 02 2015, "George Spelvin" <linux@...izon.com> wrote:
>
>> Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
>>> ... and this be part of _find_next_bit? Can find_next_bit not be simply
>>> 'return _find_next_bit(addr, size, offset, 1);', and similarly for
>>> find_next_zero_bit? Btw., passing true and false for the boolean
>>> parameter may be a little clearer.
>> Looking at the generated code, it would be better to replace the boolean
>> parameter with 0ul or ~0ul and XOR with it.  The same number of registers,
>> and saves a conditional branch.
> Nice trick. When I compiled it, gcc inlined _find_next_bit into both its
> callers, making the conditional go away completely. That was with gcc
> 4.7. When I try with 5.0, I do see _find_next_bit being compiled
> separately.
>
> With the proposed change, 4.7 also makes find_next{,_zero}_bit wrappers
> for _find_next_bit, further reducing the total size, which is a good
> thing. And, if some other version decides to still inline it, it
> should then know how to optimize the xor with 0ul or ~0ul just as well
> as when the conditional was folded away. 
>
> Yury, please also incorporate this in the next round.
>
> Rasmus
>
Ok.
What are you thinking about joining _find_next_bit and _find_next_bit_le?
They really differ in 2 lines.  It's generally good to remove duplications,
and it may decrease text size for big-endian machines. But it definitely
doesn't make code easier for reading, and maybe affects performance
after the optimization suggested by George...

(I didn't test it yet)

 29 #if !defined(find_next_bit) || !defined(find_next_zero_bit) \
 30         || (defined(BIG_ENDIAN) && \
 31                 (!defined(find_next_bit_le) || !defined(find_next_zero_bit_le)))
 32 static unsigned long _find_next_bit(const unsigned long *addr,
 33                 unsigned long nbits, unsigned long start, unsigned long flags)
 34 {
 35         unsigned long xor_mask = (flags & SET) ? 0UL : ULONG_MAX;
 36         unsigned long tmp = addr[start / BITS_PER_LONG] ^ xor_mask;
 37 
 38         /* Handle 1st word. */
 39         if (!IS_ALIGNED(start, BITS_PER_LONG)) {
 40 #ifdef BIG_ENDIAN
 41                 if (flags & LE)
 42                         tmp &= ext2_swab(HIGH_BITS_MASK(start % BITS_PER_LONG));
 43                 else
 44 #endif
 45                         tmp &= HIGH_BITS_MASK(start % BITS_PER_LONG);
 46 
 47                 start = round_down(start, BITS_PER_LONG);
 48         }
 49 
 50         while (!tmp) {
 51                 start += BITS_PER_LONG;
 52                 if (start >= nbits)
 53                         return nbits;
 54 
 55                 tmp = addr[start / BITS_PER_LONG] ^ xor_mask;
 56         }
 57 
 58 #ifdef BIG_ENDIAN
 59         if (flags & LE)
 60                 return start + __ffs(ext2_swab(tmp));
 61 
 62 #endif
 63         return start + __ffs(tmp);
 64 }
 65 #endif


--
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/