select() for delay.

select() for delay.

[madhu.subbaiah]

Hi all,

This is regarding select() system call.

Linux select() man page mentions " Some  code  calls  select with all
three sets empty, n zero, and a non-null timeout as a fairly portable
way to sleep  with  subsecond  precision".

This patch improves the sys_select() execution when used for delay. 

Kindly suggest.


--- linux-2.4.22/fs/select.c    2003-06-13 20:21:37.000000000 +0530
+++ linux/fs/select.c   2005-10-20 15:01:38.000000000 +0530
@@ -286,6 +286,29 @@ sys_select(int n, fd_set *inp, fd_set *o
        if (n < 0)
                goto out_nofds;
 

+       if ((n == 0) && (inp == NULL) && (outp == NULL) && (exp ==
NULL)) {
+                set_current_state(TASK_INTERRUPTIBLE);
+                ret = 0;
+                timeout = schedule_timeout(timeout);
+

+                if (signal_pending(current))
+                        ret = -ERESTARTNOHAND;
+

+                if (tvp && !(current->personality & STICKY_TIMEOUTS)) {
+                        time_t sec = 0, usec = 0;
+                        if (timeout) {
+                                sec = timeout / HZ;
+                                usec = timeout % HZ;
+                                usec *= (1000000 / HZ);
+                        }
+                        put_user(sec, &tvp->tv_sec);
+                        put_user(usec, &tvp->tv_usec);
+                }
+

+                current->state = TASK_RUNNING;
+                goto out_nofds;
+        }
+
        /* max_fdset can increase, so grab it once to avoid race */
        max_fdset = current->files->max_fdset;
        if (n > max_fdset)

Thanks,
Madhu K.S.

Re: select() for delay.

[Steven Rostedt]

Hi Maduhu,

On Mon, 2005-10-24 at 16:25 +0530, madhu.subbaiah@wipro.com wrote:

> +                        put_user(sec, &tvp->tv_sec);
> +                        put_user(usec, &tvp->tv_usec);

I won't comment on the rest of the patch, but this part is definitely
wrong.  The pointer tvp is a user space address and once you dereference
that pointer to get to tv_sec, you can have a fault, which might
segfault the processes.

What you really want is something like:

{
	timeval tv;

	tv.tv_sec = sec;
	tv.tv_usec = usec;
	copy_to_user(tvp, &tv, sizeof(tv));
}

-- Steve

Re: select() for delay.

[Arjan van de Ven]

On Mon, 2005-10-24 at 09:18 -0400, Steven Rostedt wrote:
> Hi Maduhu,
> 
> On Mon, 2005-10-24 at 16:25 +0530, madhu.subbaiah@wipro.com wrote:
> 
> > +                        put_user(sec, &tvp->tv_sec);
> > +                        put_user(usec, &tvp->tv_usec);
> 
> I won't comment on the rest of the patch, but this part is definitely
> wrong.  The pointer tvp is a user space address and once you dereference
> that pointer to get to tv_sec, you can have a fault, which might
> segfault the

&pointer->member  doesn't dereference the pointer, it just adds the
offset of "member" to the content of the pointer.

Re: select() for delay.

[Steven Rostedt]

On Mon, 2005-10-24 at 15:27 +0200, Arjan van de Ven wrote:
> On Mon, 2005-10-24 at 09:18 -0400, Steven Rostedt wrote:
> > Hi Maduhu,
> > 
> > On Mon, 2005-10-24 at 16:25 +0530, madhu.subbaiah@wipro.com wrote:
> > 
> > > +                        put_user(sec, &tvp->tv_sec);
> > > +                        put_user(usec, &tvp->tv_usec);
> > 
> > I won't comment on the rest of the patch, but this part is definitely
> > wrong.  The pointer tvp is a user space address and once you dereference
> > that pointer to get to tv_sec, you can have a fault, which might
> > segfault the
> 
> &pointer->member  doesn't dereference the pointer, it just adds the
> offset of "member" to the content of the pointer.
> 

Oh crap! You're right ;-)

Argh, that's what I get for replying before my first cup of coffee and
suffering jet lag.

I should have known this since I just copied over the container_of macro
to a user process, and if what I said was true, ((type *)0)->member
would never work.

Oh well, time to wake up :-)

-- Steve

Re: select() for delay.

[Madhu K.S.]

Hi All,

Someone please comment on the entire patch functionality. 
I tested this patch, it seems to work fine.

Kindly suggest.



On Mon, 2005-10-24 at 18:57, Arjan van de Ven wrote:
> On Mon, 2005-10-24 at 09:18 -0400, Steven Rostedt wrote:
> > Hi Maduhu,
> > 
> > On Mon, 2005-10-24 at 16:25 +0530, madhu.subbaiah@wipro.com wrote:
> > 
> > > +                        put_user(sec, &tvp->tv_sec);
> > > +                        put_user(usec, &tvp->tv_usec);
> > 
> > I won't comment on the rest of the patch, but this part is definitely
> > wrong.  The pointer tvp is a user space address and once you dereference
> > that pointer to get to tv_sec, you can have a fault, which might
> > segfault the
> 
> &pointer->member  doesn't dereference the pointer, it just adds the
> offset of "member" to the content of the pointer.
> 
> 

Re: select() for delay.

[Arnd Bergmann]

On Maandag 24 Oktober 2005 12:55, madhu.subbaiah@wipro.com wrote:
 
> This is regarding select() system call.
> 
> Linux select() man page mentions " Some  code  calls  select with all
> three sets empty, n zero, and a non-null timeout as a fairly portable
> way to sleep  with  subsecond  precision".

When you make a change to a system call, you should always check
if the change makes sense for the 32 bit emulation path as well.

In this case, you should definitely do the same thing to both
sys_select and compat_sys_select if this is found worthwhile.
 
> This patch improves the sys_select() execution when used for delay. 

Please describe what aspect of the syscall is improved. Is this only
speeding up the execution for the delay case while slowing down
the normal case, or do the actual semantics improve?

	Arnd <><

Re: select() for delay.

[Arjan van de Ven]

On Sun, 2005-10-30 at 20:06 +0100, Arnd Bergmann wrote:
> On Maandag 24 Oktober 2005 12:55, madhu.subbaiah@wipro.com wrote:
>  
> > This is regarding select() system call.
> > 
> > Linux select() man page mentions " Some  code  calls  select with all
> > three sets empty, n zero, and a non-null timeout as a fairly portable
> > way to sleep  with  subsecond  precision".
> 
> When you make a change to a system call, you should always check
> if the change makes sense for the 32 bit emulation path as well.
> 
> In this case, you should definitely do the same thing to both
> sys_select and compat_sys_select if this is found worthwhile.
>  
> > This patch improves the sys_select() execution when used for delay. 
> 
> Please describe what aspect of the syscall is improved. Is this only
> speeding up the execution for the delay case while slowing down
> the normal case, or do the actual semantics improve?

there is something funky about increasing the speed of a delay ;)

Re: select() for delay.

[Christopher Friesen]

Arnd Bergmann wrote:
> On Maandag 24 Oktober 2005 12:55, madhu.subbaiah@wipro.com wrote:

>>This patch improves the sys_select() execution when used for delay. 

> Please describe what aspect of the syscall is improved. Is this only
> speeding up the execution for the delay case while slowing down
> the normal case, or do the actual semantics improve?

It appears to simply speed up the delay case.

One thing that I noticed though--the patch had the following line:

if ((n == 0) && (inp == NULL) && (outp == NULL) && (exp == NULL)) {


Would it be enought to just do the following?

if (n == 0) {

Given that if any of the fd_set pointers are non-empty, "n" ought to be 
non-zero, would this be a sufficient check?

Chris