futex.c and EWOULDBLOCK vs. EAGAIN patch

futex.c and EWOULDBLOCK vs. EAGAIN patch

[Helge Deller]

Since PARISC is the only Linux architecture which has different values
for EAGAIN and EWOULDBLOCK, we are running in various issues.

One of them is, that e.g. glibc checks for EAGAIN instead of EWOULDBLOCK
(and vise versa) in nptl/pthread_mutex_trylock.c. This doesn't hurt
other architectures, but it hurts parisc.

I was planning to send the patch below to linux kernel mailing list.
But before I do it, I would like to get feedback from the kernel
hackers here on the list.

What do you think?
Is this patch useful?
Or will people just call me an idiot if I ask if this patch could be applied?
If you think it's useful, maybe another patch description would be more appropriate?

Any feedback is welcome...

Helge

-----------------------
[PATCH] futex.c: return EAGAIN instead of EWOULDBLOCK

Patch summary:
In the Linux kernel all architectures beside the PARISC architecture 
define EWOULDBLOCK as EAGAIN:

arch/sparc/include/asm/errno.h:#define  EWOULDBLOCK     EAGAIN  /* Operation would block */
arch/mips/include/asm/errno.h:#define   EWOULDBLOCK     EAGAIN  /* Operation would block */
arch/alpha/include/asm/errno.h:#define  EWOULDBLOCK     EAGAIN  /* Operation would block */
include/asm-generic/errno.h:#define     EWOULDBLOCK     EAGAIN  /* Operation would block */

The only exception is PARISC, which tries to keep HP-UX compatibility:
arch/parisc/include/asm/errno.h:#define EWOULDBLOCK     246     /* Operation would block (Linux returns EAGAIN) */

This difference leads to various problems with userspace, which often 
thinks that EWOULDBLOCK and EGAIN are of the same value. To avoid problems
userspace sometimes has to check the return value for both values.

The following patch cleans up the Linux kernel futex implementation to
always return EAGAIN instead of EWOULDBLOCK.
Binary compatibility for all architectures beside PARISC is still kept,
but on PARISC this will ease life a lot.

Signed-off-by: Helge Deller <deller@gmx.de>


diff --git a/kernel/futex.c b/kernel/futex.c
index e7a35f1..28d14ff 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1732,7 +1732,7 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
  *
  * Returns:
  *  0 - uaddr contains val and hb has been locked
- * <1 - -EFAULT or -EWOULDBLOCK (uaddr does not contain val) and hb is unlcoked
+ * <1 - -EFAULT or -EAGAIN (uaddr does not contain val) and hb is unlcoked
  */
 static int futex_wait_setup(u32 __user *uaddr, u32 val, int fshared,
 			   struct futex_q *q, struct futex_hash_bucket **hb)
@@ -1784,7 +1784,7 @@ retry_private:
 
 	if (uval != val) {
 		queue_unlock(q, *hb);
-		ret = -EWOULDBLOCK;
+		ret = -EAGAIN;
 	}
 
 out:
@@ -1969,7 +1969,7 @@ retry_private:
 	else {
 		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
 		/* Fixup the trylock return value: */
-		ret = ret ? 0 : -EWOULDBLOCK;
+		ret = ret ? 0 : -EAGAIN;
 	}
 
 	spin_lock(q.lock_ptr);
@@ -2154,7 +2154,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
 		plist_del(&q->list, &q->list.plist);
 
 		/* Handle spurious wakeups gracefully */
-		ret = -EWOULDBLOCK;
+		ret = -EAGAIN;
 		if (timeout && !timeout->task)
 			ret = -ETIMEDOUT;
 		else if (signal_pending(current))
@@ -2196,7 +2196,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
  * 7) timeout
  * 8) other lock acquisition failure
  *
- * If 6, return -EWOULDBLOCK (restarting the syscall would do the same).
+ * If 6, return -EAGAIN (restarting the syscall would do the same).
  *
  * If 4 or 7, we cleanup and return with -ETIMEDOUT.
  *
@@ -2318,10 +2318,10 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, int fshared,
 		 * We've already been requeued, but cannot restart by calling
 		 * futex_lock_pi() directly. We could restart this syscall, but
 		 * it would detect that the user space "val" changed and return
-		 * -EWOULDBLOCK.  Save the overhead of the restart and return
-		 * -EWOULDBLOCK directly.
+		 * -EAGAIN.  Save the overhead of the restart and return
+		 * -EAGAIN directly.
 		 */
-		ret = -EWOULDBLOCK;
+		ret = -EAGAIN;
 	}
 
 out_put_keys:

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Grant Grundler]

On Mon, May 10, 2010 at 09:41:40PM +0200, Helge Deller wrote:
> Since PARISC is the only Linux architecture which has different values
> for EAGAIN and EWOULDBLOCK, we are running in various issues.
> 
> One of them is, that e.g. glibc checks for EAGAIN instead of EWOULDBLOCK
> (and vise versa) in nptl/pthread_mutex_trylock.c. This doesn't hurt
> other architectures, but it hurts parisc.
> 
> I was planning to send the patch below to linux kernel mailing list.
> But before I do it, I would like to get feedback from the kernel
> hackers here on the list.
> 
> What do you think?

Do it.
We kept them distinct to enable support for HPUX binaries.
This is never going to happen.

> Is this patch useful?

Yes.

> Or will people just call me an idiot if I ask if this patch could be applied?
> If you think it's useful, maybe another patch description would be more appropriate?

Could we just equate like other arcthitectures so the kernel only ever
returns one of those and not both?

> Any feedback is welcome...

Thanks for bringing this up! :)

cheers,
grant

> 
> Helge
> 
> -----------------------
> [PATCH] futex.c: return EAGAIN instead of EWOULDBLOCK
> 
> Patch summary:
> In the Linux kernel all architectures beside the PARISC architecture 
> define EWOULDBLOCK as EAGAIN:
> 
> arch/sparc/include/asm/errno.h:#define  EWOULDBLOCK     EAGAIN  /* Operation would block */
> arch/mips/include/asm/errno.h:#define   EWOULDBLOCK     EAGAIN  /* Operation would block */
> arch/alpha/include/asm/errno.h:#define  EWOULDBLOCK     EAGAIN  /* Operation would block */
> include/asm-generic/errno.h:#define     EWOULDBLOCK     EAGAIN  /* Operation would block */
> 
> The only exception is PARISC, which tries to keep HP-UX compatibility:
> arch/parisc/include/asm/errno.h:#define EWOULDBLOCK     246     /* Operation would block (Linux returns EAGAIN) */
> 
> This difference leads to various problems with userspace, which often 
> thinks that EWOULDBLOCK and EGAIN are of the same value. To avoid problems
> userspace sometimes has to check the return value for both values.
> 
> The following patch cleans up the Linux kernel futex implementation to
> always return EAGAIN instead of EWOULDBLOCK.
> Binary compatibility for all architectures beside PARISC is still kept,
> but on PARISC this will ease life a lot.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index e7a35f1..28d14ff 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1732,7 +1732,7 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
>   *
>   * Returns:
>   *  0 - uaddr contains val and hb has been locked
> - * <1 - -EFAULT or -EWOULDBLOCK (uaddr does not contain val) and hb is unlcoked
> + * <1 - -EFAULT or -EAGAIN (uaddr does not contain val) and hb is unlcoked
>   */
>  static int futex_wait_setup(u32 __user *uaddr, u32 val, int fshared,
>  			   struct futex_q *q, struct futex_hash_bucket **hb)
> @@ -1784,7 +1784,7 @@ retry_private:
>  
>  	if (uval != val) {
>  		queue_unlock(q, *hb);
> -		ret = -EWOULDBLOCK;
> +		ret = -EAGAIN;
>  	}
>  
>  out:
> @@ -1969,7 +1969,7 @@ retry_private:
>  	else {
>  		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
>  		/* Fixup the trylock return value: */
> -		ret = ret ? 0 : -EWOULDBLOCK;
> +		ret = ret ? 0 : -EAGAIN;
>  	}
>  
>  	spin_lock(q.lock_ptr);
> @@ -2154,7 +2154,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
>  		plist_del(&q->list, &q->list.plist);
>  
>  		/* Handle spurious wakeups gracefully */
> -		ret = -EWOULDBLOCK;
> +		ret = -EAGAIN;
>  		if (timeout && !timeout->task)
>  			ret = -ETIMEDOUT;
>  		else if (signal_pending(current))
> @@ -2196,7 +2196,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
>   * 7) timeout
>   * 8) other lock acquisition failure
>   *
> - * If 6, return -EWOULDBLOCK (restarting the syscall would do the same).
> + * If 6, return -EAGAIN (restarting the syscall would do the same).
>   *
>   * If 4 or 7, we cleanup and return with -ETIMEDOUT.
>   *
> @@ -2318,10 +2318,10 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, int fshared,
>  		 * We've already been requeued, but cannot restart by calling
>  		 * futex_lock_pi() directly. We could restart this syscall, but
>  		 * it would detect that the user space "val" changed and return
> -		 * -EWOULDBLOCK.  Save the overhead of the restart and return
> -		 * -EWOULDBLOCK directly.
> +		 * -EAGAIN.  Save the overhead of the restart and return
> +		 * -EAGAIN directly.
>  		 */
> -		ret = -EWOULDBLOCK;
> +		ret = -EAGAIN;
>  	}
>  
>  out_put_keys:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Carlos O'Donell]

On Fri, May 14, 2010 at 3:20 PM, Grant Grundler
<grundler@parisc-linux.org> wrote:
> On Mon, May 10, 2010 at 09:41:40PM +0200, Helge Deller wrote:
>> Since PARISC is the only Linux architecture which has different values
>> for EAGAIN and EWOULDBLOCK, we are running in various issues.
>>
>> One of them is, that e.g. glibc checks for EAGAIN instead of EWOULDBLOCK
>> (and vise versa) in nptl/pthread_mutex_trylock.c. This doesn't hurt
>> other architectures, but it hurts parisc.
>>
>> I was planning to send the patch below to linux kernel mailing list.
>> But before I do it, I would like to get feedback from the kernel
>> hackers here on the list.
>>
>> What do you think?
>
> Do it.
> We kept them distinct to enable support for HPUX binaries.
> This is never going to happen.

Grant, does this mean you endorse purging the kernel of all uses of
EWOULDBLOCK (with the exception of the header file used by userspace)?

FWIW I think this is the right approach.

>> Is this patch useful?
>
> Yes.
>
>> Or will people just call me an idiot if I ask if this patch could be applied?
>> If you think it's useful, maybe another patch description would be more appropriate?
>
> Could we just equate like other arcthitectures so the kernel only ever
> returns one of those and not both?

There are ABI issues that I would like to discuss before you send your
patch upstream.

Once you change the kernel EWOULDBLOCK to be equal to EAGAIN, you have
broken the ABI.

A kernel syscall that used to return EWOULDBLOCK, now returns EAGAIN.

An application that checked only for EWOULDBLOCK might fail now that
EAGAIN is being returned.

Is this a concern? I don't think it is. The POSIX standard, in all
cases I could find, says the functions return "EAGAIN or EWOULDBLOCK",
meaning that a conforming program must check for both.

The alternative to Helge's patch is a glibc patch to check the return
of *every* syscall and convert EWOULDBLOCK to EAGAIN. This approach
has a cost to our syscall fast path. Therefore I am not as inclined to
follow this approach.

Cheers,
Carlos.

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Kyle McMartin]

On Sat, May 15, 2010 at 07:24:54AM -0400, Carlos O'Donell wrote:
> An application that checked only for EWOULDBLOCK might fail now that
> EAGAIN is being returned.
> 
> Is this a concern? I don't think it is. The POSIX standard, in all
> cases I could find, says the functions return "EAGAIN or EWOULDBLOCK",
> meaning that a conforming program must check for both.
> 

It's a two instruction penalty(*) in the kernel syscall return path, just
do it there. I've got a box at Red Hat with all the source to the whole
damn world on it we run greps against to look for dumb stuff like memcpy
bugs. I'll toss a grep for EAGAIN and not EWOULDBLOCK and vice versa and
see how much stuff might b0rk.

We discussed this a couple years ago, and it ended up being a complete
rats nest of error returns that needed fixing.

regards, Kyle.

*: CMPI,N(=), LDI, maybe a third since I don't remember how sign
extension works off the top of my head.

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Grant Grundler]

On Sat, May 15, 2010 at 01:27:49PM -0400, Kyle McMartin wrote:
> It's a two instruction penalty(*) in the kernel syscall return path, just
> do it there. I've got a box at Red Hat with all the source to the whole
> damn world on it we run greps against to look for dumb stuff like memcpy
> bugs.

Why look for it? Just change the kernel to not ever return one of them.

>  I'll toss a grep for EAGAIN and not EWOULDBLOCK and vice versa and
> see how much stuff might b0rk.

Please do...I'd be curious to see which programs might be affected and
if we care.

> We discussed this a couple years ago, and it ended up being a complete
> rats nest of error returns that needed fixing.
> 
> regards, Kyle.
> 
> *: CMPI,N(=), LDI, maybe a third since I don't remember how sign
> extension works off the top of my head.

Mispredicted branches cost something like 20-25 cycles. I forgot now but I
do know they aren't cheap.

I'd rather not add more hacks for this they are unlikely to ever
be removed again. I'd rather have status quo than add some hack
to the syscall return path.

thanks,
grant

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Kyle McMartin]

On Sat, May 15, 2010 at 04:59:52PM -0600, Grant Grundler wrote:
> On Sat, May 15, 2010 at 01:27:49PM -0400, Kyle McMartin wrote:
> > It's a two instruction penalty(*) in the kernel syscall return path, just
> > do it there. I've got a box at Red Hat with all the source to the whole
> > damn world on it we run greps against to look for dumb stuff like memcpy
> > bugs.
> 
> Why look for it? Just change the kernel to not ever return one of them.
> 

Because the failure case of just changing the #define in the kernel is a
runtime error for old code.

> >  I'll toss a grep for EAGAIN and not EWOULDBLOCK and vice versa and
> > see how much stuff might b0rk.
> 
> Please do...I'd be curious to see which programs might be affected and
> if we care.
> 
> > We discussed this a couple years ago, and it ended up being a complete
> > rats nest of error returns that needed fixing.
> > 
> > regards, Kyle.
> > 
> > *: CMPI,N(=), LDI, maybe a third since I don't remember how sign
> > extension works off the top of my head.
> 
> Mispredicted branches cost something like 20-25 cycles. I forgot now but I
> do know they aren't cheap.
> 

That's not a branch, Grant.

> I'd rather not add more hacks for this they are unlikely to ever
> be removed again. I'd rather have status quo than add some hack
> to the syscall return path.
> 

The alternative is to rebuild all of userspace, again.

regards, Kyle

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Grant Grundler]

On Sat, May 15, 2010 at 07:03:59PM -0400, Kyle McMartin wrote:
...
> > > *: CMPI,N(=), LDI, maybe a third since I don't remember how sign
> > > extension works off the top of my head.
> > 
> > Mispredicted branches cost something like 20-25 cycles. I forgot now but I
> > do know they aren't cheap.
> 
> That's not a branch, Grant.

DOh! sorry...you are right. /o\
I was totally missing the instruction nullification.

> > I'd rather not add more hacks for this they are unlikely to ever
> > be removed again. I'd rather have status quo than add some hack
> > to the syscall return path.
> 
> The alternative is to rebuild all of userspace, again.

All?
If I generate a list of debian packages that look at EWOULDBLOCK or EAGAIN,
couldn't we just regenerate those packages once the kernel change is in
place and the kernel header files pushed to debian experimental?
There can't be that many packages.

And of those that do, I believe most time the old binaries will generally
work since EWOULDBLOCK code paths are unlikely to get exercised. Or is
there evidence to the contrary?

thanks
grant

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Kyle McMartin]

On Sun, May 16, 2010 at 10:12:07AM -0600, Grant Grundler wrote:
> If I generate a list of debian packages that look at EWOULDBLOCK or EAGAIN,
> couldn't we just regenerate those packages once the kernel change is in
> place and the kernel header files pushed to debian experimental?
> There can't be that many packages.
> 

Sure, but what about people running the old stable version of Debian or
something? We lose the ability to have (probably) useful userspace
running when kernels get bisected across the flag version.

> And of those that do, I believe most time the old binaries will generally
> work since EWOULDBLOCK code paths are unlikely to get exercised. Or is
> there evidence to the contrary?
> 

That's what we need to find out. :) Because it will "just work" on most
other operating systems I've been able to quickly grep for, people might
have been negligent.

I think Helge should definitely submit his very fine patches to
glibc/kernel wherever. As John points out, I looked up POSIX tonight and
as it stands they don't violate it technically (since the value is the
same) but they symbollically do. :)

regards, Kyle

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Carlos O'Donell]

On Sat, May 15, 2010 at 1:27 PM, Kyle McMartin <kyle@mcmartin.ca> wrote:
> On Sat, May 15, 2010 at 07:24:54AM -0400, Carlos O'Donell wrote:
>> An application that checked only for EWOULDBLOCK might fail now that
>> EAGAIN is being returned.
>>
>> Is this a concern? I don't think it is. The POSIX standard, in all
>> cases I could find, says the functions return "EAGAIN or EWOULDBLOCK",
>> meaning that a conforming program must check for both.
>>
>
> It's a two instruction penalty(*) in the kernel syscall return path, just
> do it there. I've got a box at Red Hat with all the source to the whole
> damn world on it we run greps against to look for dumb stuff like memcpy
> bugs. I'll toss a grep for EAGAIN and not EWOULDBLOCK and vice versa and
> see how much stuff might b0rk.

Is there any benefit to doing it in userspace?

As opposed to changing the kernel to define EGAIN == EWOULDBLOCK?

Both changes break ABI.

The userspace change makes the syscall return path a couple of
instructions slower.

The kernel change might break the future emulation of hpux application
on parisc linux?

> We discussed this a couple years ago, and it ended up being a complete
> rats nest of error returns that needed fixing.

If we set EAGAIN=EWOULDBLOCK in the kernel for parisc, would that be hard?

I'm just trying to understand and weigh the options.

Cheers,
Carlos.

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[Grant Grundler]

On Sat, May 15, 2010 at 07:24:54AM -0400, Carlos O'Donell wrote:
> On Fri, May 14, 2010 at 3:20 PM, Grant Grundler
> <grundler@parisc-linux.org> wrote:
> > On Mon, May 10, 2010 at 09:41:40PM +0200, Helge Deller wrote:
> >> Since PARISC is the only Linux architecture which has different values
> >> for EAGAIN and EWOULDBLOCK, we are running in various issues.
> >>
> >> One of them is, that e.g. glibc checks for EAGAIN instead of EWOULDBLOCK
> >> (and vise versa) in nptl/pthread_mutex_trylock.c. This doesn't hurt
> >> other architectures, but it hurts parisc.
> >>
> >> I was planning to send the patch below to linux kernel mailing list.
> >> But before I do it, I would like to get feedback from the kernel
> >> hackers here on the list.
> >>
> >> What do you think?
> >
> > Do it.
> > We kept them distinct to enable support for HPUX binaries.
> > This is never going to happen.
> 
> Grant, does this mean you endorse purging the kernel of all uses of
> EWOULDBLOCK (with the exception of the header file used by userspace)?
> 
> FWIW I think this is the right approach.

Yes, I would. Any approach to eliminate the difference is fine with me.

BTW, "purging" here just means redefining to EAGAIN so the original
value that we've been using would no longer be used anywhere.

> 
> >> Is this patch useful?
> >
> > Yes.
> >
> >> Or will people just call me an idiot if I ask if this patch could be applied?
> >> If you think it's useful, maybe another patch description would be more appropriate?
> >
> > Could we just equate like other arcthitectures so the kernel only ever
> > returns one of those and not both?
> 
> There are ABI issues that I would like to discuss before you send your
> patch upstream.
> 
> Once you change the kernel EWOULDBLOCK to be equal to EAGAIN, you have
> broken the ABI.
> 
> A kernel syscall that used to return EWOULDBLOCK, now returns EAGAIN.
> 
> An application that checked only for EWOULDBLOCK might fail now that
> EAGAIN is being returned.
> 
> Is this a concern? I don't think it is. The POSIX standard, in all
> cases I could find, says the functions return "EAGAIN or EWOULDBLOCK",
> meaning that a conforming program must check for both.

I totally understand it would break the existing API. But having them
different has it's own set of problems since some applications do not
check for both.

> The alternative to Helge's patch is a glibc patch to check the return
> of *every* syscall and convert EWOULDBLOCK to EAGAIN. This approach
> has a cost to our syscall fast path. Therefore I am not as inclined to
> follow this approach.

Agreed. Just equate them in the kernel and we don't have to change any
other kernel code and I see no reason for glibc to trap them either.
Yes, there will be some minor fallout...but now we can saw we are compliant
with every other linux architecture in this regard.

thanks,
grant

> 
> Cheers,
> Carlos.

Re: futex.c and EWOULDBLOCK vs. EAGAIN patch

[John David Anglin]

> Since PARISC is the only Linux architecture which has different values
> for EAGAIN and EWOULDBLOCK, we are running in various issues.
> 
> One of them is, that e.g. glibc checks for EAGAIN instead of EWOULDBLOCK
> (and vise versa) in nptl/pthread_mutex_trylock.c. This doesn't hurt
> other architectures, but it hurts parisc.

Glibc should be fixed so that it is complient with IEEE Std 1003.1.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)