Re: Hugh number of file descriptor checks

Re: Hugh number of file descriptor checks

[Peter Stephenson]

(Moved to zsh-workers.  This is definitely not a user issue.)

Phil Pennock wrote:
> Meanwhile, max_zsh_fd is the largest fd that has been allocated by the
> shell itself, rather than inherited.
> 
> zopenmax() is used to close all file-descriptors except some which are
> being kept, and to determine the size of the fdtable which is nominally
> what sets max_zsh_fd.  zopenmax() returns OPEN_MAX in the event that
> sysconf() isn't available.
> 
> 
> Questions for the list:
>  Why not just pick a magic value like 64 or 32 and, if
>  sysconf(_SC_OPEN_MAX) is larger than that, use the magic value?

Given that this is basically there to determine fdtable_size, and in
fact we'll try to reallocate that if needed (so trying too hard to work
out what it should be seems a bit pointless) that sounds perfectly
workable to me.

>  Why do
>  we need to know the largest fd already in use?  Does fdtable have to be
>  able to handle the largest fd already open, if not opened by zsh?

Well, unless I'm missing something fdtable doesn't get initialised to
take account of anything already open anyway, so I don't see how this
can be important to the code as written.  I think the shell generally is
written to use whatever fd's the system gives it above 9.

I think it would also make more sense for closeallelse() to use
fdtable_size rather than call zopenmax(), and for zopenmax() not to
bother caching openmax.  It seems to me that's one parameter too many
and it's not really what it claims to be anyway... and if we increase
fdtable_size, surely using openmax in closeallelse() is plain wrong?

How about the following?  I also tried to tidy up the rellocation of
fdtable a bit.

Index: Src/compat.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/compat.c,v
retrieving revision 1.18
diff -p -u -r1.18 compat.c
--- Src/compat.c	19 Mar 2009 15:00:22 -0000	1.18
+++ Src/compat.c	19 Feb 2010 09:57:17 -0000
@@ -187,18 +187,23 @@ zpathmax(char *dir)
 #endif /* 0 */
 
 #ifdef HAVE_SYSCONF
-/* This is replaced by a macro from system.h if not HAVE_SYSCONF.    *
- * 0 is returned by sysconf if _SC_OPEN_MAX is unavailable;          *
- * -1 is returned on error                                           *
- *                                                                   *
- * Neither of these should happen, but resort to OPEN_MAX rather     *
- * than return 0 or -1 just in case.                                 */
+/*
+ * This is replaced by a macro from system.h if not HAVE_SYSCONF.
+ * 0 is returned by sysconf if _SC_OPEN_MAX is unavailable;
+ * -1 is returned on error
+ *
+ * Neither of these should happen, but resort to OPEN_MAX rather
+ * than return 0 or -1 just in case.
+ *
+ * We'll limit the open maximum to ZSH_INITIAL_OPEN_MAX to
+ * avoid probing ridiculous numbers of file descriptors.
+ */
 
 /**/
 mod_export long
 zopenmax(void)
 {
-    static long openmax = 0;
+    long openmax = 0;
 
     if (openmax < 1) {
 	if ((openmax = sysconf(_SC_OPEN_MAX)) < 1) {
@@ -212,6 +217,8 @@ zopenmax(void)
 	     * So, report the maximum of OPEN_MAX or the largest open *
 	     * descriptor (is there a better way to find that?).      */
 	    long i, j = OPEN_MAX;
+	    if (openmax > ZSH_INITIAL_OPEN_MAX)
+		openmax = ZSH_INITIAL_OPEN_MAX;
 	    for (i = j; i < openmax; i += (errno != EINTR)) {
 		errno = 0;
 		if (fcntl(i, F_GETFL, 0) < 0 &&
@@ -771,7 +778,7 @@ mk_wcwidth(wchar_t ucs)
 
   /* if we arrive here, ucs is not a combining or C0/C1 control character */
 
-  return 1 + 
+  return 1 +
     (ucs >= 0x1100 &&
      (ucs <= 0x115f ||                    /* Hangul Jamo init. consonants */
       ucs == 0x2329 || ucs == 0x232a ||
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.175
diff -p -u -r1.175 exec.c
--- Src/exec.c	9 Feb 2010 13:58:33 -0000	1.175
+++ Src/exec.c	19 Feb 2010 09:57:17 -0000
@@ -1928,7 +1928,7 @@ closeallelse(struct multio *mn)
     int i, j;
     long openmax;
 
-    openmax = zopenmax();
+    openmax = fdtable_size;
 
     for (i = 0; i < openmax; i++)
 	if (mn->pipe != i) {
Index: Src/system.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/system.h,v
retrieving revision 1.55
diff -p -u -r1.55 system.h
--- Src/system.h	1 Aug 2009 04:20:35 -0000	1.55
+++ Src/system.h	19 Feb 2010 09:57:17 -0000
@@ -304,16 +304,22 @@ struct timezone {
 # endif
 #endif
 
+/*
+ * The number of file descriptors we'll allocate initially.
+ * We will reallocate later if necessary.
+ */
+#define ZSH_INITIAL_OPEN_MAX 64
 #ifndef OPEN_MAX
 # ifdef NOFILE
 #  define OPEN_MAX NOFILE
 # else
    /* so we will just pick something */
-#  define OPEN_MAX 64
+#  define OPEN_MAX ZSH_INITIAL_OPEN_MAX
 # endif
 #endif
 #ifndef HAVE_SYSCONF
-# define zopenmax() ((long) OPEN_MAX)
+# define zopenmax() ((long) (OPEN_MAX > ZSH_INITIAL_OPEN_MAX ? \
+			     ZSH_INITIAL_OPEN_MAX : OPEN_MAX))
 #endif
 
 #ifdef HAVE_FCNTL_H
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.237
diff -p -u -r1.237 utils.c
--- Src/utils.c	9 Feb 2010 14:24:01 -0000	1.237
+++ Src/utils.c	19 Feb 2010 09:57:17 -0000
@@ -1621,6 +1621,27 @@ adjustwinsize(int from)
     }
 }
 
+/*
+ * Ensure the fdtable is large enough for fd, and that the
+ * maximum fd is set appropriately.
+ */
+static void
+check_fd_table(int fd)
+{
+    if (fd <= max_zsh_fd)
+	return;
+
+    if (fd >= fdtable_size) {
+	int old_size = fdtable_size;
+	while (fd >= fdtable_size)
+	    fdtable = zrealloc(fdtable,
+			       (fdtable_size *= 2)*sizeof(*fdtable));
+	memset(fdtable + old_size, 0,
+	       (fdtable_size - old_size) * sizeof(*fdtable));
+    }
+    max_zsh_fd = fd;
+}
+
 /* Move a fd to a place >= 10 and mark the new fd in fdtable.  If the fd *
  * is already >= 10, it is not moved.  If it is invalid, -1 is returned. */
 
@@ -1644,12 +1665,7 @@ movefd(int fd)
 	fd = fe;
     }
     if(fd != -1) {
-	if (fd > max_zsh_fd) {
-	    while (fd >= fdtable_size)
-		fdtable = zrealloc(fdtable,
-				   (fdtable_size *= 2)*sizeof(*fdtable));
-	    max_zsh_fd = fd;
-	}
+	check_fd_table(fd);
 	fdtable[fd] = FDT_INTERNAL;
     }
     return fd;
@@ -1669,12 +1685,12 @@ redup(int x, int y)
     if(x < 0)
 	zclose(y);
     else if (x != y) {
-	while (y >= fdtable_size)
-	    fdtable = zrealloc(fdtable, (fdtable_size *= 2)*sizeof(*fdtable));
-	if (dup2(x, y) == -1)
+	if (dup2(x, y) == -1) {
 	    ret = -1;
-	if ((fdtable[y] = fdtable[x]) && y > max_zsh_fd)
-	    max_zsh_fd = y;
+	} else {
+	    check_fd_table(y);
+	    fdtable[y] = fdtable[x];
+	}
 	zclose(x);
     }
 
-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Hugh number of file descriptor checks

[Phil Pennock]

On 2010-02-19 at 10:45 +0000, Peter Stephenson wrote:
> How about the following?  I also tried to tidy up the rellocation of
> fdtable a bit.

Looks sane.  :)
-Phil

Re: Hugh number of file descriptor checks

[Bart Schaefer]

Sorry to be a bit late responding to this, I was traveling ...

On Feb 19, 10:45am, Peter Stephenson wrote:
} 
} How about the following?  I also tried to tidy up the rellocation of
} fdtable a bit.
} 
}  /**/
}  mod_export long
}  zopenmax(void)
}  {
} -    static long openmax = 0;
} +    long openmax = 0;
}  
}      if (openmax < 1) {

Did you really mean to remove "static" there?  With that removed, the
"if (openmax < 1)" will ALWAYS be true, so there's no point in testing
it -- but the intention of having openmax be static was so that once
it's determined, we know what it is and never probe it again.

That is, on certain platforms I think you've traded startup-time
inefficiency for later run-time inefficiency.

Re: Hugh number of file descriptor checks

[Peter Stephenson]

On Sun, 21 Feb 2010 09:11:52 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Sorry to be a bit late responding to this, I was traveling ...
> 
> On Feb 19, 10:45am, Peter Stephenson wrote:
> } 
> } How about the following?  I also tried to tidy up the rellocation of
> } fdtable a bit.
> } 
> }  /**/
> }  mod_export long
> }  zopenmax(void)
> }  {
> } -    static long openmax = 0;
> } +    long openmax = 0;
> }  
> }      if (openmax < 1) {
> 
> Did you really mean to remove "static" there?

Yes.

>  With that removed, the
> "if (openmax < 1)" will ALWAYS be true, so there's no point in testing
> it 

You're right, that can go.

> -- but the intention of having openmax be static was so that once
> it's determined, we know what it is and never probe it again.
> 
> That is, on certain platforms I think you've traded startup-time
> inefficiency for later run-time inefficiency.

It should now only ever be called at initialisation, since I've removed
the only other call to it (unless you can see otherwise).

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Hugh number of file descriptor checks

[Bart Schaefer]

On Feb 21,  6:34pm, Peter Stephenson wrote:
}
} [zopenmax()] should now only ever be called at initialisation, since
} I've removed the only other call to it (unless you can see otherwise).

Aha.  You're right, that looks OK.