RFC: poll change

RFC: poll change

[Tim Hockin]

hey all, I have a request from someone to investigate this.

poll() currently does not allow you to pass more fd's than you have open. 
At first this seems reasonable.  However, I have seen now at least one app
that takes advantage of a behavior found on some poll() implementations (I
don't have the standard available..).

On these systems, you may pass in as many fds as you want, and any
structure with the fd member set to < 0 is ignored.  This allows apps to
save on allocating and re-allocating, or sorting the fd arrays.

What would the feeling be if I implemented this for Linux?

Tim
-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
thockin@sun.com

Re: RFC: poll change

[David S. Miller]

   From: Tim Hockin <thockin@sun.com>
   Date: Tue, 14 Aug 2001 14:08:40 -0700

   poll() currently does not allow you to pass more fd's than you have open. 

Tim, please check the latest sources, this has been fixed
in 2.4.x for several months.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change - nevermind

[Tim Hockin]

Tim Hockin wrote:

> poll() currently does not allow you to pass more fd's than you have 

I'll shut up - I was looking at 2.2.x source.


-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
thockin@sun.com

Re: RFC: poll change

[Herbert Xu]

David S. Miller <davem@redhat.com> wrote:
>   From: Tim Hockin <thockin@sun.com>
>   Date: Tue, 14 Aug 2001 14:08:40 -0700

>   poll() currently does not allow you to pass more fd's than you have open. 

> Tim, please check the latest sources, this has been fixed
> in 2.4.x for several months.

Hmm, it still seems to be wrong:

        /* Do a sanity check on nfds ... */
        if (nfds > NR_OPEN)
                return -EINVAL;

        if (nfds > current->files->max_fds)
                nfds = current->files->max_fds;

The second if statement should be removed.  And it might be better to use
current->rlim[RLIMIT_NOFILE].rlim_cur instead of NR_OPEN.
-- 
Debian GNU/Linux 2.2 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: RFC: poll change

[David S. Miller]

   From: Herbert Xu <herbert@gondor.apana.org.au>
   Date: Wed, 15 Aug 2001 08:38:02 +1000

   Hmm, it still seems to be wrong:
 ...

           if (nfds > current->files->max_fds)
                   nfds = current->files->max_fds;
   
   The second if statement should be removed.  And it might be better to use
   current->rlim[RLIMIT_NOFILE].rlim_cur instead of NR_OPEN.

It has to be limited to "max_fds", that is how many files we may
legally address in current->files!

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change - nevermind

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 03:03:07PM -0700, Tim Hockin wrote:
> Tim Hockin wrote:
> 
> > poll() currently does not allow you to pass more fd's than you have 
> 
> I'll shut up - I was looking at 2.2.x source.

btw, it is also fixed in my 2.2 tree since 2.2.20pre8aa1:

	ftp://ftp.kernel.org/pub/linux/kernel/people/andrea/kernels/v2.2/2.2.20pre8aa1/00_poll-max_fds-1

Andrea

Re: RFC: poll change

[David S. Miller]

   From: Tim Hockin <thockin@sun.com>
   Date: Tue, 14 Aug 2001 16:53:43 -0700

   The standard says negative fd's are ignored.  We get that right.  What we
   are left with is an overly paranoid check against max_fds.  This check
   should go away.  You should be able to pass in up to your rlimit fds, and
   let negative ones (holes or tails) be ignored.
   
I am saying there is no problem.

In both cases, for a properly written application we ignore the
invalid fds.  The behavior is identical both before and after
your change, so there is no reason to make it.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[Tim Hockin]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

"David S. Miller" wrote:
 
>    The standard also says that any pollfd with (fd < 0) is ignored.  Holes are
>    explicitly ALLOWED.
> 
> Dude, it ignores negative fds, check fs/select.c:do_pollfd()

Right - we're running in circles.

The standard says negative fd's are ignored.  We get that right.  What we
are left with is an overly paranoid check against max_fds.  This check
should go away.  You should be able to pass in up to your rlimit fds, and
let negative ones (holes or tails) be ignored.

I'm attaching a patch :).

Am I still not making the problem clear?

-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
thockin@sun.com

[-- Attachment #2: select.diff --]
[-- Type: text/plain, Size: 583 bytes --]

Index: fs/select.c
===================================================================
RCS file: /home/cvs/linux-2.4/fs/select.c,v
retrieving revision 1.5
diff -u -r1.5 select.c
--- fs/select.c	2001/07/09 23:10:25	1.5
+++ fs/select.c	2001/08/14 23:47:46
@@ -416,11 +416,8 @@
 	int nchunks, nleft;
 
 	/* Do a sanity check on nfds ... */
-	if (nfds > NR_OPEN)
+	if (nfds > current->rlim[RLIMIT_NOFILE].rlim_cur)
 		return -EINVAL;
-
-	if (nfds > current->files->max_fds)
-		nfds = current->files->max_fds;
 
 	if (timeout) {
 		/* Careful about overflow in the intermediate values */

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 03:42:33PM -0700, David S. Miller wrote:
>    From: Herbert Xu <herbert@gondor.apana.org.au>
>    Date: Wed, 15 Aug 2001 08:38:02 +1000
> 
>    Hmm, it still seems to be wrong:
>  ...
> 
>            if (nfds > current->files->max_fds)
>                    nfds = current->files->max_fds;
>    
>    The second if statement should be removed.  And it might be better to use
>    current->rlim[RLIMIT_NOFILE].rlim_cur instead of NR_OPEN.
> 
> It has to be limited to "max_fds", that is how many files we may
> legally address in current->files!

current->files only matters for the contents of the pollfd struct, not
for the number of pollfd structures passed on, removing such two lines
shouldn't destabilize anything, I think it's just a matter of API,
reading SuS it seems not to imply we have to truncate it to the max_fds
but OTOH the feedback I had was just happy with the 2.4 fix (so I didn't
even checked if the 2.4 fix was fully compliant or not..).

Andrea

Re: RFC: poll change

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Wed, 15 Aug 2001 02:03:03 +0200

   but OTOH the feedback I had was just happy with the 2.4 fix (so I didn't
   even checked if the 2.4 fix was fully compliant or not..).

This is why I'm saying there is no practical reason to make
this change.

The current code can actually improve performance, avoiding needlessly
scanning fd==-1 entries.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[David S. Miller]

   From: Tim Hockin <thockin@sun.com>
   Date: Tue, 14 Aug 2001 17:09:00 -0700

   But for an application that (imho) is poorly written but IS COMPLIANT, it
   fails.
   
   This program is compliant, if your ulimit -n is maxxed out at 1048576.

This program is stupid.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 04:53:20PM -0700, David S. Miller wrote:
> invalid fds.  The behavior is identical both before and after
> your change, so there is no reason to make it.

actually I can see a case where it makes difference: if you sends 1024
entries to poll with the last entry of the array polling for one of the
allocated file descriptors (all previous 1023 entries are negative) and
the last fd allocated is ID 255.

Andrea

Re: RFC: poll change

[Tim Hockin]

"David S. Miller" wrote:
> 
>    From: Tim Hockin <thockin@sun.com>
>    Date: Tue, 14 Aug 2001 16:53:43 -0700
> 
>    The standard says negative fd's are ignored.  We get that right.  What we
>    are left with is an overly paranoid check against max_fds.  This check
>    should go away.  You should be able to pass in up to your rlimit fds, and
>    let negative ones (holes or tails) be ignored.
>
> I am saying there is no problem.
> 
> In both cases, for a properly written application we ignore the
> invalid fds.  The behavior is identical both before and after
> your change, so there is no reason to make it.

But for an application that (imho) is poorly written but IS COMPLIANT, it
fails.

This program is compliant, if your ulimit -n is maxxed out at 1048576.

I'm not saying it is good design, but it is compliant.  The patch hurts
nothing and makes poll() that little bit more friendly.



#include <stdio.h>
#include <sys/poll.h>
#include <errno.h>
#include <string.h>

int
main(void)
{
        static struct pollfd ar[1024*1024];
        int size = sizeof(ar)/sizeof(*ar);
        int i;

        for (i = 0; i < size; i++)
                ar[i].fd = -1;
        ar[1000000].fd = 0;
        ar[1000000].events = POLLIN;

        i = poll(ar, size, -1);
        printf("return = %d (%s)\n", i, i?strerror(errno):"success");

        return 0;
}


-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Cobalt Server Appliances
thockin@sun.com

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 04:53:43PM -0700, Tim Hockin wrote:
> -	if (nfds > NR_OPEN)
> +	if (nfds > current->rlim[RLIMIT_NOFILE].rlim_cur)

Here SuS speaks about OPEN_MAX, not sure if OPEN_MAX corresponds to the
current ulimit or to the absolute maximum (to me it sounds more like our
NR_OPEN).

Andrea

Re: RFC: poll change

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Wed, 15 Aug 2001 02:11:10 +0200

   On Tue, Aug 14, 2001 at 04:53:43PM -0700, Tim Hockin wrote:
   > -	if (nfds > NR_OPEN)
   > +	if (nfds > current->rlim[RLIMIT_NOFILE].rlim_cur)
   
   Here SuS speaks about OPEN_MAX, not sure if OPEN_MAX corresponds to the
   current ulimit or to the absolute maximum (to me it sounds more like our
   NR_OPEN).

Right, and our equivalent is "NR_OPEN".

Ok, I think I see what you and Tim are trying to say.
I'm thinking in a select() minded way which is why I didn't
understand :-)

Yeah, the check can be removed, but anyone who cares about
performance won't pass in huge arrays of -1 entries if only
the low few pollfd entries are actually useful.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 05:06:20PM -0700, David S. Miller wrote:
>    From: Andrea Arcangeli <andrea@suse.de>
>    Date: Wed, 15 Aug 2001 02:03:03 +0200
> 
>    but OTOH the feedback I had was just happy with the 2.4 fix (so I didn't
>    even checked if the 2.4 fix was fully compliant or not..).
> 
> This is why I'm saying there is no practical reason to make
> this change.

Probably there isn't pratical reason agreed.

> The current code can actually improve performance, avoiding needlessly
> scanning fd==-1 entries.

you assume the entries above max_fds are set to -1, while strictly
speaking they don't need to.

Andrea

Re: RFC: poll change

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Wed, 15 Aug 2001 02:16:48 +0200
   
   > The current code can actually improve performance, avoiding needlessly
   > scanning fd==-1 entries.
   
   you assume the entries above max_fds are set to -1, while strictly
   speaking they don't need to.

Right, this was my core misunderstanding, see my most
recent mail.

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 05:16:09PM -0700, David S. Miller wrote:
> Yeah, the check can be removed, but anyone who cares about
> performance won't pass in huge arrays of -1 entries if only
> the low few pollfd entries are actually useful.

yes, on the same lines I'm not sure who needs the -1 entries in first
place since one of the major points of poll is to avoid useless browse
of non interesting entries. I'm scared people uses poll just to
workaround the 1024 bit limit on the fdset structures in glibc and to
generate arrays larger than 1024. I hope it's just a resource management
issue, where only a few entries are sometime set to -1 and so they can
avoid to duplicate the polltable for very minor difference in their
utilization or for slow path cases.

Andrea

RE: RFC: poll change

[David Schwartz]

>    From: Herbert Xu <herbert@gondor.apana.org.au>
>    Date: Wed, 15 Aug 2001 08:38:02 +1000
>
>    Hmm, it still seems to be wrong:
>  ...
>
>            if (nfds > current->files->max_fds)
>                    nfds = current->files->max_fds;
>
>    The second if statement should be removed.  And it might be
> better to use
>    current->rlim[RLIMIT_NOFILE].rlim_cur instead of NR_OPEN.
>
> It has to be limited to "max_fds", that is how many files we may
> legally address in current->files!

	Though stupid, I believe it is perfectly legal to, say, have one socket
open and poll for it with two separate entries, one checking readability and
one checking writability.

	DS

Re: RFC: poll change

[Andrea Arcangeli]

On Tue, Aug 14, 2001 at 05:16:09PM -0700, David S. Miller wrote:
>    From: Andrea Arcangeli <andrea@suse.de>
>    Date: Wed, 15 Aug 2001 02:11:10 +0200
> 
>    On Tue, Aug 14, 2001 at 04:53:43PM -0700, Tim Hockin wrote:
>    > -	if (nfds > NR_OPEN)
>    > +	if (nfds > current->rlim[RLIMIT_NOFILE].rlim_cur)
>    
>    Here SuS speaks about OPEN_MAX, not sure if OPEN_MAX corresponds to the
>    current ulimit or to the absolute maximum (to me it sounds more like our
>    NR_OPEN).
> 
> Right, and our equivalent is "NR_OPEN".

I was backporting the new version to 2.2 and I noticed that by using
NR_OPEN an luser will actually be able to lock into RAM something of the
order of the dozen mbytes per process. So I'm wondering that it would be
saner to use the rlimit instead, after all I don't see much of a value
to use NR_OPEN instead of the rlimit (even if strictly speaking SuS asks
us to use NR_OPEN). Any weird program (if any) that would depend on
NR_OPEN instead of the rlimit can be easily fixed with a one liner at
most. So I guess I'd be more happy with the rlimit instead of NR_OPEN.
Comments?

Andrea

Re: RFC: poll change

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Wed, 15 Aug 2001 16:13:18 +0200

   I was backporting the new version to 2.2 and I noticed that by using
   NR_OPEN an luser will actually be able to lock into RAM something of the
   order of the dozen mbytes per process.

He can do this with page tables too :-)

Later,
David S. Miller
davem@redhat.com

Re: RFC: poll change

[Andrea Arcangeli]

On Wed, Aug 15, 2001 at 07:24:19AM -0700, David S. Miller wrote:
>    From: Andrea Arcangeli <andrea@suse.de>
>    Date: Wed, 15 Aug 2001 16:13:18 +0200
> 
>    I was backporting the new version to 2.2 and I noticed that by using
>    NR_OPEN an luser will actually be able to lock into RAM something of the
>    order of the dozen mbytes per process.
> 
> He can do this with page tables too :-)

Since 2.2 we have the free_pgtables to release the pagetables under
unused pgd slots, that was used to work pretty well last time I checked.

Andrea

Re: RFC: poll change

[Hugh Dickins]

On Wed, 15 Aug 2001, Andrea Arcangeli wrote:
> 
> Since 2.2 we have the free_pgtables to release the pagetables under
> unused pgd slots, that was used to work pretty well last time I checked.

Funny you mention that: I noticed a while back that actually
it doesn't work well with i386 PAE - presumably looks for an empty 1GB.

Hugh

Re: RFC: poll change

[Andrea Arcangeli]

On Wed, Aug 15, 2001 at 05:30:04PM +0100, Hugh Dickins wrote:
> On Wed, 15 Aug 2001, Andrea Arcangeli wrote:
> > 
> > Since 2.2 we have the free_pgtables to release the pagetables under
> > unused pgd slots, that was used to work pretty well last time I checked.
> 
> Funny you mention that: I noticed a while back that actually
> it doesn't work well with i386 PAE - presumably looks for an empty 1GB.

correct, we'd need the equivalent for the pmd too, this applies to the
other 3-level (or more) pagetables archs too.

Andrea