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.
Hi Maduhu,
On Mon, 2005-10-24 at 16:25 +0530, [email protected] 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
On Mon, 2005-10-24 at 09:18 -0400, Steven Rostedt wrote:
> Hi Maduhu,
>
> On Mon, 2005-10-24 at 16:25 +0530, [email protected] 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.
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, [email protected] 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
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, [email protected] 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.
>
>
On Maandag 24 Oktober 2005 12:55, [email protected] 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 <><
On Sun, 2005-10-30 at 20:06 +0100, Arnd Bergmann wrote:
> On Maandag 24 Oktober 2005 12:55, [email protected] 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 ;)
Arnd Bergmann wrote:
> On Maandag 24 Oktober 2005 12:55, [email protected] 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