Hi,
the following patch now returns -EBADF in sys_select()/do_select()
if all specified file-descriptors are bad (e.g. were closed by another
thread).
Without this patch, the for-loop in do_select() won't stop if there are no
valid files any more. It stops only if a specified timeout occured or a
signal arrived.
If there are no objections on this patch, I would also create a patch for
kernel 2.6 and the poll() code. I didn't have a closer look at this yet.
Armin
--- linux/fs/select.c_orig 2004-03-02 19:06:44.000000000 +0100
+++ linux/fs/select.c 2004-03-03 09:25:24.000000000 +0100
@@ -183,6 +183,8 @@
wait = NULL;
retval = 0;
for (;;) {
+ int file_err = 1;
+
set_current_state(TASK_INTERRUPTIBLE);
for (i = 0 ; i < n; i++) {
unsigned long bit = BIT(i);
@@ -199,6 +201,7 @@
i /* The fd*/,
__timeout,
NULL);
+ file_err = 0;
mask = DEFAULT_POLLMASK;
if (file->f_op && file->f_op->poll)
mask = file->f_op->poll(file, wait);
@@ -227,6 +230,10 @@
retval = table.error;
break;
}
+ if (file_err) {
+ retval = -EBADF;
+ break;
+ }
__timeout = schedule_timeout(__timeout);
}
current->state = TASK_RUNNING;
Hi,
On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> --- linux/fs/select.c_orig 2004-03-02 19:06:44.000000000 +0100
> +++ linux/fs/select.c 2004-03-03 09:25:24.000000000 +0100
> @@ -183,6 +183,8 @@
> wait = NULL;
> retval = 0;
> for (;;) {
> + int file_err = 1;
> +
Just a thought, select() is often performance-critical, and adding one more
variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
set retval to -EBADF above and avoid using file_err ?
> set_current_state(TASK_INTERRUPTIBLE);
> for (i = 0 ; i < n; i++) {
> unsigned long bit = BIT(i);
> @@ -199,6 +201,7 @@
> i /* The fd*/,
> __timeout,
> NULL);
> + file_err = 0;
There you would put retval=0
> mask = DEFAULT_POLLMASK;
> if (file->f_op && file->f_op->poll)
> mask = file->f_op->poll(file, wait);
> @@ -227,6 +230,10 @@
> retval = table.error;
> break;
> }
> + if (file_err) {
> + retval = -EBADF;
> + break;
> + }
And there : if (retval == -EBADF) break;
> __timeout = schedule_timeout(__timeout);
> }
> current->state = TASK_RUNNING;
Just a thought anyway, I've not read the complete function.
Cheers,
Willy
On Thu, 4 Mar 2004, Willy Tarreau wrote:
> Hi,
>
> On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> > --- linux/fs/select.c_orig 2004-03-02 19:06:44.000000000 +0100
> > +++ linux/fs/select.c 2004-03-03 09:25:24.000000000 +0100
> > @@ -183,6 +183,8 @@
> > wait = NULL;
> > retval = 0;
> > for (;;) {
> > + int file_err = 1;
> > +
>
> Just a thought, select() is often performance-critical, and adding one more
> variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
> set retval to -EBADF above and avoid using file_err ?
retval cannot be used for that, it may get changed in the loop.
Anyway, I don't see how your proposal would do better performance?
My patch just adds a new variable on the stack, which should not make any
difference in performance. And later, it is the same if the new or another
variable gets changed or checked.
Armin
On Thu, Mar 04, 2004 at 10:20:41AM +0100, Armin Schindler wrote:
> retval cannot be used for that, it may get changed in the loop.
OK
> Anyway, I don't see how your proposal would do better performance?
> My patch just adds a new variable on the stack, which should not make any
> difference in performance. And later, it is the same if the new or another
> variable gets changed or checked.
It is possible that in current code the, compiler is able to allocate a
register for each variable used inside the loop, while it may not be able
anymore with an additionnal variable. But that's just speculation anyway.
Cheers,
Willy
On Thu, 4 Mar 2004, Armin Schindler wrote:
> On Thu, 4 Mar 2004, Willy Tarreau wrote:
> > Hi,
> >
> > On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> > > --- linux/fs/select.c_orig 2004-03-02 19:06:44.000000000 +0100
> > > +++ linux/fs/select.c 2004-03-03 09:25:24.000000000 +0100
> > > @@ -183,6 +183,8 @@
> > > wait = NULL;
> > > retval = 0;
> > > for (;;) {
> > > + int file_err = 1;
> > > +
> >
> > Just a thought, select() is often performance-critical, and adding one more
> > variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
> > set retval to -EBADF above and avoid using file_err ?
>
> retval cannot be used for that, it may get changed in the loop.
>
> Anyway, I don't see how your proposal would do better performance?
> My patch just adds a new variable on the stack, which should not make any
> difference in performance. And later, it is the same if the new or another
> variable gets changed or checked.
Curiosity: Does SuS/POSIX define behaviour for "all fds are closed" ?
--- 2.6/fs/select.c 2004-03-14 14:28:28.000000000 +0100
+++ build-2.6/fs/select.c 2004-03-14 19:08:57.000000000 +0100
@@ -223,25 +223,25 @@
break;
if (!(bit & all_bits))
continue;
+ mask = DEFAULT_POLLMASK;
file = fget(i);
if (file) {
f_op = file->f_op;
- mask = DEFAULT_POLLMASK;
if (f_op && f_op->poll)
mask = (*f_op->poll)(file, retval ? NULL : wait);
fput(file);
- if ((mask & POLLIN_SET) && (in & bit)) {
- res_in |= bit;
- retval++;
- }
- if ((mask & POLLOUT_SET) && (out & bit)) {
- res_out |= bit;
- retval++;
- }
- if ((mask & POLLEX_SET) && (ex & bit)) {
- res_ex |= bit;
- retval++;
- }
+ }
+ if ((mask & POLLIN_SET) && (in & bit)) {
+ res_in |= bit;
+ retval++;
+ }
+ if ((mask & POLLOUT_SET) && (out & bit)) {
+ res_out |= bit;
+ retval++;
+ }
+ if ((mask & POLLEX_SET) && (ex & bit)) {
+ res_ex |= bit;
+ retval++;
}
}
if (res_in)
On Sun, 14 Mar 2004, Manfred Spraul wrote:
> Marcelo wrote:
>
> >>
> >> Anyway, I don't see how your proposal would do better performance?
> >> My patch just adds a new variable on the stack, which should not make any
> >> difference in performance. And later, it is the same if the new or another
> >> variable gets changed or checked.
> >
> >Curiosity: Does SuS/POSIX define behaviour for "all fds are closed" ?
> >
> >
> I'd interpret SuS that a closed fd is ready for reading and writing:
> From the select page:
> <<<
> A descriptor shall be considered ready for reading when a call to an
> input function with O_NONBLOCK clear would not block, whether or not the
> function would transfer data successfully. (The function might return
> data, an end-of-file indication, or an error other than one indicating
> that it is blocked, and in each of these cases the descriptor shall be
> considered ready for reading.)
> <<<
> read(fd,,) will return immediately with EBADF, thus the fd is ready.
>
> But that's a grey area, especially if you close the fd during the select
> call. For example HP UX just kills the current process if an fd that is
> polled is closed by overwriting it with dup2. I didn't test select, but
> I'd expect a similar behavior.
>
> Armin: did you compare the Linux behavior with other unices? Are there
> other unices that return EBADF for select() if all fds are closed?
No, I didn't compare yet, but I could not find any definition on that. It
really seems to be a "grey area".
> Attached is an untested proposal, against 2.6, but I'm not sure if it's
> really a good idea to change the current code - it might break existing
> apps.
This patch should also work on 2.4 and looks good to me, if "ready" should
be returned instead of EBADF. I don't think this would break existing
apps. Without such a patch, the app would sleep forever unless a signal
arrives. If any app depends on that behavior, I think it is bad coded.
Armin