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 - [email protected] > > 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);
> _______________________________________________
> [email protected] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[email protected]"
pgpE294LaSxZb.pgp
Description: PGP signature
