subject:"fdgrowtable\(\) cleanup"

Re: fdgrowtable() cleanup

2012-09-20 Thread Konstantin Belousov

On Wed, Sep 19, 2012 at 09:04:30PM +0200, Dag-Erling Sm??rgrav wrote:
 Konstantin Belousov kostik...@gmail.com writes:
  Dag-Erling Sm??rgrav d...@des.no writes:
   I assume you mean assignments, not calculations.  I trust the compiler
   to move them to where they are needed - a trivial optimization with SSA.
  It is a dereference before the assignment, so I perceive it as the
  calculation. No, I do not think that compiler can move the dereference,
  because there are many sequential points between the calculation and
  use of the result.
 
 Sequence points are a language feature and are only meaningful in the
 translation phase.  Once the code is in SSA form or some other
 equivalent intermediate representation, the compiler can see that the
 variables are only used in one specific case and move the assignments
 inside that block.  In fact, it will probably optimize them away,
 because they are completely unnecessary - I added them solely for
 readability after Niclas called my attention to the fact that it is
 almost impossible to understand fdgrowtable() at a first reading.
Compiler cannot change the semantic of the program regardless of the phase
of compilation the change happens at.

Compiler that would reorder reads from the global memory across
sequential points seems to be not disallowed by c99, but it certainly
contradicts to our semantic of the lock releases, which the compiler
cannot infer from the non-inlined function calls. Malloc call is
the sequential point and does have the release semantic FWIW.
 
   Correct, thanks for pointing it out.  The easiest solution is to place
   the struct freetable between the file array and the flag array.
  As I know, for all ABIs FreeBSD run on, the structure alignment is the
  alignment of the most requiring member. You would introduce very tacit
  dependency between struct file and struct freetable.
 
 The existing code *already* places the struct freetable immediately
 after the struct file array.  What I'm proposing now is to leave the
 struct freetable where it was but move the fileflags array so they don't
 overlap.  The fileflags array is actually a char[] and has no alignment
 requirement.
Ok.

 
 Memory usage will not increase, because the code already allocates
 additional space for the struct freetable to make sure it will fit even
 if onfiles  sizeof(struct freetable).
 
 BTW, I just noticed that there is some dead code in fdgrowtable():
 
   nnfiles = NDSLOTS(nfd) * NDENTRIES; /* round up */
   if (nnfiles = onfiles)
   /* the table is already large enough */
   return;
 
 /* ... */
 
   /* allocate new bitmaps if necessary */
   if (NDSLOTS(nnfiles)  NDSLOTS(onfiles))
   nmap = malloc(NDSLOTS(nnfiles) * NDSLOTSIZE,
   M_FILEDESC, M_ZERO | M_WAITOK);
   else
   nmap = NULL;
 
 Since neither nnflags nor onflags are modified between these two chunks
 of code, the condition in the second if will always be true.

You mean that new bitmap shall be always allocated, making the
nmap = NULL assignment not needed ? I agree. It would also make the
code in the last if () executed unconditionally.


pgpxF1S7g8LNi.pgp
Description: PGP signature

Re: fdgrowtable() cleanup

2012-09-19 Thread Konstantin Belousov

On Tue, Sep 18, 2012 at 05:46:23PM +0200, Dag-Erling Sm??rgrav wrote:
 The patch below my signature improves the legibility of fdgrowtable(),
 and adds comments explaining the hairier bits.  The only functional
 change is that the code no longer overwrites the old fileflags array
 when the old table is placed on the freelist; instead, it uses the space
 actually set aside for that purpose.
 
 (I assume that the old behavior was harmless, since it has persisted for
 decades, but it was certainly confusing.)
 
 The slightly repetitive nature of the new code is intentional.
 
 DES
 -- 
 Dag-Erling Sm??rgrav - d...@des.no
 
 Index: sys/kern/kern_descrip.c
 ===
 --- sys/kern/kern_descrip.c   (revision 240654)
 +++ sys/kern/kern_descrip.c   (working copy)
 @@ -148,11 +148,6 @@
  #define  NDSLOTS(x)  (((x) + NDENTRIES - 1) / NDENTRIES)
  
  /*
 - * Storage required per open file descriptor.
 - */
 -#define OFILESIZE (sizeof(struct file *) + sizeof(char))
 -
 -/*
   * Storage to hold unused ofiles that need to be reclaimed.
   */
  struct freetable {
 @@ -1436,7 +1431,7 @@
   struct freetable *fo;
   struct file **ntable;
   struct file **otable;
 - char *nfileflags;
 + char *nfileflags, *ofileflags;
   int nnfiles, onfiles;
   NDSLOTTYPE *nmap;
  
 @@ -1447,33 +1442,46 @@
  
   /* compute the size of the new table */
   onfiles = fdp-fd_nfiles;
 + otable = fdp-fd_ofiles;
 + ofileflags = fdp-fd_ofileflags;
These two new calculations could be unused if the function return early.

   nnfiles = NDSLOTS(nfd) * NDENTRIES; /* round up */
   if (nnfiles = onfiles)
   /* the table is already large enough */
   return;
  
 - /* allocate a new table and (if required) new bitmaps */
 - ntable = malloc((nnfiles * OFILESIZE) + sizeof(struct freetable),
 + /*
 +  * Allocate a new table.  We need enough space for a) the file
 +  * entries themselves, b) the file flags, and c) the struct
 +  * freetable we will use when we decommission the table and place
 +  * it on the freelist.
 +  */
 + ntable = malloc(nnfiles * sizeof(*ntable) +
 + nnfiles * sizeof(*nfileflags) +
 + sizeof(struct freetable),
   M_FILEDESC, M_ZERO | M_WAITOK);
Please use the horizontal space less lavishly.

I think that this calculation, as well as fo calculation below, does not
take a required alignment of struct freetable into consideration.

   nfileflags = (char *)ntable[nnfiles];
 +
 + /* allocate new bitmaps if necessary */
   if (NDSLOTS(nnfiles)  NDSLOTS(onfiles))
   nmap = malloc(NDSLOTS(nnfiles) * NDSLOTSIZE,
   M_FILEDESC, M_ZERO | M_WAITOK);
   else
   nmap = NULL;
  
 - bcopy(fdp-fd_ofiles, ntable, onfiles * sizeof(*ntable));
 - bcopy(fdp-fd_ofileflags, nfileflags, onfiles);
 - otable = fdp-fd_ofiles;
 + /* copy the old data over and point at the new tables */
 + bcopy(otable, ntable, onfiles * sizeof(*otable));
 + bcopy(ofileflags, nfileflags, onfiles * sizeof(*ofileflags));
   fdp-fd_ofileflags = nfileflags;
   fdp-fd_ofiles = ntable;
 +
   /*
* We must preserve ofiles until the process exits because we can't
* be certain that no threads have references to the old table via
* _fget().
*/
   if (onfiles  NDFILE) {
 - fo = (struct freetable *)otable[onfiles];
 + fo = (struct freetable *)(char *)otable +
 + onfiles * sizeof(*otable) + onfiles * sizeof(*ofileflags);
   fdp0 = (struct filedesc0 *)fdp;
   fo-ft_table = otable;
   SLIST_INSERT_HEAD(fdp0-fd_free, fo, ft_next);
 ___
 freebsd-hackers@freebsd.org mailing list
 http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
 To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


pgpE294LaSxZb.pgp
Description: PGP signature

Re: fdgrowtable() cleanup

2012-09-19 Thread Dag-Erling Smørgrav

Konstantin Belousov kostik...@gmail.com writes:
 Dag-Erling Smørgrav d...@des.no writes:
  +   otable = fdp-fd_ofiles;
  +   ofileflags = fdp-fd_ofileflags;
 These two new calculations could be unused if the function return early.

I assume you mean assignments, not calculations.  I trust the compiler
to move them to where they are needed - a trivial optimization with SSA.

  +   ntable = malloc(nnfiles * sizeof(*ntable) +
  +   nnfiles * sizeof(*nfileflags) +
  +   sizeof(struct freetable),
  M_FILEDESC, M_ZERO | M_WAITOK);
 Please use the horizontal space less lavishly.

I was aiming for readability, not compatibility with equipment that went
out of use before I was born.

 I think that this calculation, as well as fo calculation below, does
 not take a required alignment of struct freetable into consideration.

Correct, thanks for pointing it out.  The easiest solution is to place
the struct freetable between the file array and the flag array.

DES
-- 
Dag-Erling Smørgrav - d...@des.no
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

Re: fdgrowtable() cleanup

2012-09-19 Thread Konstantin Belousov

On Wed, Sep 19, 2012 at 02:58:02PM +0200, Dag-Erling Sm??rgrav wrote:
 Konstantin Belousov kostik...@gmail.com writes:
  Dag-Erling Sm??rgrav d...@des.no writes:
   + otable = fdp-fd_ofiles;
   + ofileflags = fdp-fd_ofileflags;
  These two new calculations could be unused if the function return early.
 
 I assume you mean assignments, not calculations.  I trust the compiler
 to move them to where they are needed - a trivial optimization with SSA.
It is a dereference before the assignment, so I perceive it as the
calculation. No, I do not think that compiler can move the dereference,
because there are many sequential points between the calculation and
use of the result.

 
   + ntable = malloc(nnfiles * sizeof(*ntable) +
   + nnfiles * sizeof(*nfileflags) +
   + sizeof(struct freetable),
 M_FILEDESC, M_ZERO | M_WAITOK);
  Please use the horizontal space less lavishly.
 
 I was aiming for readability, not compatibility with equipment that went
 out of use before I was born.
Well, my eyes can only see so many lines in the emacs window. So if you
referenced older equipment meaning me being born before you, so be it.
:)

 
  I think that this calculation, as well as fo calculation below, does
  not take a required alignment of struct freetable into consideration.
 
 Correct, thanks for pointing it out.  The easiest solution is to place
 the struct freetable between the file array and the flag array.

As I know, for all ABIs FreeBSD run on, the structure alignment is the
alignment of the most requiring member. You would introduce very tacit
dependency between struct file and struct freetable.


pgpA1ORGQL5xf.pgp
Description: PGP signature

Re: fdgrowtable() cleanup

2012-09-19 Thread Dag-Erling Smørgrav

Konstantin Belousov kostik...@gmail.com writes:
 Dag-Erling Smørgrav d...@des.no writes:
  I assume you mean assignments, not calculations.  I trust the compiler
  to move them to where they are needed - a trivial optimization with SSA.
 It is a dereference before the assignment, so I perceive it as the
 calculation. No, I do not think that compiler can move the dereference,
 because there are many sequential points between the calculation and
 use of the result.

Sequence points are a language feature and are only meaningful in the
translation phase.  Once the code is in SSA form or some other
equivalent intermediate representation, the compiler can see that the
variables are only used in one specific case and move the assignments
inside that block.  In fact, it will probably optimize them away,
because they are completely unnecessary - I added them solely for
readability after Niclas called my attention to the fact that it is
almost impossible to understand fdgrowtable() at a first reading.

  Correct, thanks for pointing it out.  The easiest solution is to place
  the struct freetable between the file array and the flag array.
 As I know, for all ABIs FreeBSD run on, the structure alignment is the
 alignment of the most requiring member. You would introduce very tacit
 dependency between struct file and struct freetable.

The existing code *already* places the struct freetable immediately
after the struct file array.  What I'm proposing now is to leave the
struct freetable where it was but move the fileflags array so they don't
overlap.  The fileflags array is actually a char[] and has no alignment
requirement.

Memory usage will not increase, because the code already allocates
additional space for the struct freetable to make sure it will fit even
if onfiles  sizeof(struct freetable).

BTW, I just noticed that there is some dead code in fdgrowtable():

nnfiles = NDSLOTS(nfd) * NDENTRIES; /* round up */
if (nnfiles = onfiles)
/* the table is already large enough */
return;

/* ... */

/* allocate new bitmaps if necessary */
if (NDSLOTS(nnfiles)  NDSLOTS(onfiles))
nmap = malloc(NDSLOTS(nnfiles) * NDSLOTSIZE,
M_FILEDESC, M_ZERO | M_WAITOK);
else
nmap = NULL;

Since neither nnflags nor onflags are modified between these two chunks
of code, the condition in the second if will always be true.

DES
-- 
Dag-Erling Smørgrav - d...@des.no
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

fdgrowtable() cleanup

2012-09-18 Thread Dag-Erling Smørgrav

The patch below my signature improves the legibility of fdgrowtable(),
and adds comments explaining the hairier bits.  The only functional
change is that the code no longer overwrites the old fileflags array
when the old table is placed on the freelist; instead, it uses the space
actually set aside for that purpose.

(I assume that the old behavior was harmless, since it has persisted for
decades, but it was certainly confusing.)

The slightly repetitive nature of the new code is intentional.

DES
-- 
Dag-Erling Smørgrav - d...@des.no

Index: sys/kern/kern_descrip.c
===
--- sys/kern/kern_descrip.c (revision 240654)
+++ sys/kern/kern_descrip.c (working copy)
@@ -148,11 +148,6 @@
 #defineNDSLOTS(x)  (((x) + NDENTRIES - 1) / NDENTRIES)
 
 /*
- * Storage required per open file descriptor.
- */
-#define OFILESIZE (sizeof(struct file *) + sizeof(char))
-
-/*
  * Storage to hold unused ofiles that need to be reclaimed.
  */
 struct freetable {
@@ -1436,7 +1431,7 @@
struct freetable *fo;
struct file **ntable;
struct file **otable;
-   char *nfileflags;
+   char *nfileflags, *ofileflags;
int nnfiles, onfiles;
NDSLOTTYPE *nmap;
 
@@ -1447,33 +1442,46 @@
 
/* compute the size of the new table */
onfiles = fdp-fd_nfiles;
+   otable = fdp-fd_ofiles;
+   ofileflags = fdp-fd_ofileflags;
nnfiles = NDSLOTS(nfd) * NDENTRIES; /* round up */
if (nnfiles = onfiles)
/* the table is already large enough */
return;
 
-   /* allocate a new table and (if required) new bitmaps */
-   ntable = malloc((nnfiles * OFILESIZE) + sizeof(struct freetable),
+   /*
+* Allocate a new table.  We need enough space for a) the file
+* entries themselves, b) the file flags, and c) the struct
+* freetable we will use when we decommission the table and place
+* it on the freelist.
+*/
+   ntable = malloc(nnfiles * sizeof(*ntable) +
+   nnfiles * sizeof(*nfileflags) +
+   sizeof(struct freetable),
M_FILEDESC, M_ZERO | M_WAITOK);
nfileflags = (char *)ntable[nnfiles];
+
+   /* allocate new bitmaps if necessary */
if (NDSLOTS(nnfiles)  NDSLOTS(onfiles))
nmap = malloc(NDSLOTS(nnfiles) * NDSLOTSIZE,
M_FILEDESC, M_ZERO | M_WAITOK);
else
nmap = NULL;
 
-   bcopy(fdp-fd_ofiles, ntable, onfiles * sizeof(*ntable));
-   bcopy(fdp-fd_ofileflags, nfileflags, onfiles);
-   otable = fdp-fd_ofiles;
+   /* copy the old data over and point at the new tables */
+   bcopy(otable, ntable, onfiles * sizeof(*otable));
+   bcopy(ofileflags, nfileflags, onfiles * sizeof(*ofileflags));
fdp-fd_ofileflags = nfileflags;
fdp-fd_ofiles = ntable;
+
/*
 * We must preserve ofiles until the process exits because we can't
 * be certain that no threads have references to the old table via
 * _fget().
 */
if (onfiles  NDFILE) {
-   fo = (struct freetable *)otable[onfiles];
+   fo = (struct freetable *)(char *)otable +
+   onfiles * sizeof(*otable) + onfiles * sizeof(*ofileflags);
fdp0 = (struct filedesc0 *)fdp;
fo-ft_table = otable;
SLIST_INSERT_HEAD(fdp0-fd_free, fo, ft_next);
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

Re: fdgrowtable() cleanup

Re: fdgrowtable() cleanup

Re: fdgrowtable() cleanup

Re: fdgrowtable() cleanup

Re: fdgrowtable() cleanup

fdgrowtable() cleanup

6 matches

Site Navigation

Mail list logo

Footer information