Re: [PATCH] Re: today's futex changes

[Jamie Lokier <jamie@shareable.org>]

Rusty Russell wrote:
> +	u32 hash = jhash2((u32*)&key->both.word,

Have you checked the code size?

That does more work and has more code than is needed, especially on
32-bit archs.  On 32-bit, jhash_3words() is much better because it
reduces to a single call to __jhash_mix(), instead of the two done by
jhash2 (only one is required for good hashing afaict).

It is probably worth adding a jhash_3longs() to jhash.h, which does
one call to __hash_mix() on 32-bit, two calls on 64-bit, and avoids
the loop in both cases.

[ Aside: For hashing individual integers, I prefer to use Thomas Wang's:

	http://www.concentric.net/~Ttwang/tech/inthash.htm

  He mentions Jenkin's function, and derived an integer mixing function
  from correspondence with Jenkins.

  For hashing 3 words together, Jenkins' hash does seem a bit more
  compact - if you don't call __jhash_mix() multiple times that is! ]

> -	if (unlikely((vma->vm_flags & (VM_IO|VM_READ)) != VM_READ))
> -		return (vma->vm_flags & VM_IO) ? -EPERM : -EACCES;
> +	if (unlikely(vma->vm_flags & VM_IO))
> +		return -EPERM;
> +	if (unlikely(vma->vm_flags & (VM_READ|VM_WRITE)) != (VM_READ|VM_WRITE))
> +		return -EACCES;

Is there a good reason to disallow read-only waiters?
I agree with Hugh that it seems like a regression.

> +	/* A spurious wakeup.  Should never happen. */
> +	BUG();

:)

The rest of your changes seem fine.  I particularly appreciate your
grammatical improvements to my comment :)

-- Jamie