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
