fdgrowtable() cleanup

[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 - des at 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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20120919/0336739a/attachment.pgp