Re: 2.6.17-rc4-mm3

[Michael Buesch <mb@bu3sch.de>]

On Monday 22 May 2006 13:38, you wrote:
> On Mon, 22 May 2006 13:25:10 +0200, Michael Buesch said:
> > On Monday 22 May 2006 13:15, you wrote:
> 
> > > It looks to me line the old code stayed in a while() loop in rng_dev_read
> > > until it had fulfilled the read request (including possibly multiple
> > > calls to need_resched() and friends).  The new code will bail on an -EAGAIN
> > > as soon as the *first* poll fails, rather than waiting until something
> > > is available - even if it is NOT flagged O_NONBLOCK.
> > 
> > Yeah. That is how it works. I am wondering why userspace doesn't
> > simply retry, if it receives an EAGAIN.
> > Should we return ERESTARTSYS or something like that instead?
> 
> That's not the way it worked in previous kernels, and it's not the way that
> the current rng-utils RPM in Fedora expects it to work.
> 
> Here's a patch that makes it work the way it used to.  Adding the test
> for O_NONBLOCK is the biggie - the old code did a resched test at that
> point in the loop, so I added it here too.
> 
> --- linux-2.6.17-rc4-mm3/drivers/char/hw_random/core.c.rnd_fix	2006-05-22 07:23:34.000000000 -0400
> +++ linux-2.6.17-rc4-mm3/drivers/char/hw_random/core.c	2006-05-22 07:22:29.000000000 -0400
> @@ -125,7 +125,7 @@ static ssize_t rng_dev_read(struct file 
>  		mutex_unlock(&rng_mutex);
>  
>  		err = -EAGAIN;
> -		if (!bytes_read)
> +		if (filp->f_flags & O_NONBLOCK && !bytes_read)
>  			goto out;
>  
>  		err = -EFAULT;
> @@ -138,6 +138,9 @@ static ssize_t rng_dev_read(struct file 
>  			data >>= 8;
>  		}
>  
> +		if (need_resched())
> +                        schedule_timeout_interruptible(1);
> +

Andrew's comment on this:

	What's going on with the need_resched() tricks in there?  (Unobvious, needs
	a comment).  From my reading, it'll cause a caller to this function to hang
	for arbitrary periods of time if something if causing heavy scheduling
	pressure.

So I decided to remove it and return -EAGAIN, so userspace can retry.
But seems like it it does not. I thought glibc would handle that.