Date: Wed, 19 Sep 2012 09:10:05 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Dag-Erling Sm??rgrav <des@des.no> Cc: hackers@freebsd.org Subject: Re: fdgrowtable() cleanup Message-ID: <20120919061005.GR37286@deviant.kiev.zoral.com.ua> In-Reply-To: <86wqzr8hbk.fsf@ds4.des.no> References: <86wqzr8hbk.fsf@ds4.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
--0p6fkQojZ86Dx7oG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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.
>=20
> (I assume that the old behavior was harmless, since it has persisted for
> decades, but it was certainly confusing.)
>=20
> The slightly repetitive nature of the new code is intentional.
>=20
> DES
> --=20
> Dag-Erling Sm??rgrav - des@des.no
>=20
> Index: sys/kern/kern_descrip.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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)
> =20
> /*
> - * 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;
> =20
> @@ -1447,33 +1442,46 @@
> =20
> /* compute the size of the new table */
> onfiles =3D fdp->fd_nfiles;
> + otable =3D fdp->fd_ofiles;
> + ofileflags =3D fdp->fd_ofileflags;
These two new calculations could be unused if the function return early.
> nnfiles =3D NDSLOTS(nfd) * NDENTRIES; /* round up */
> if (nnfiles <=3D onfiles)
> /* the table is already large enough */
> return;
> =20
> - /* allocate a new table and (if required) new bitmaps */
> - ntable =3D 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 =3D 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 =3D (char *)&ntable[nnfiles];
> +
> + /* allocate new bitmaps if necessary */
> if (NDSLOTS(nnfiles) > NDSLOTS(onfiles))
> nmap =3D malloc(NDSLOTS(nnfiles) * NDSLOTSIZE,
> M_FILEDESC, M_ZERO | M_WAITOK);
> else
> nmap =3D NULL;
> =20
> - bcopy(fdp->fd_ofiles, ntable, onfiles * sizeof(*ntable));
> - bcopy(fdp->fd_ofileflags, nfileflags, onfiles);
> - otable =3D 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 =3D nfileflags;
> fdp->fd_ofiles =3D 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 =3D (struct freetable *)&otable[onfiles];
> + fo =3D (struct freetable *)(char *)otable +
> + onfiles * sizeof(*otable) + onfiles * sizeof(*ofileflags);
> fdp0 =3D (struct filedesc0 *)fdp;
> fo->ft_table =3D 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-unsubscribe@freebsd.org"
--0p6fkQojZ86Dx7oG
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (FreeBSD)
iEUEARECAAYFAlBZYb0ACgkQC3+MBN1Mb4ieowCVGSDK2XOcFD9sTCgsq/ErfCBo
rwCeOxO292mbQ0ClxXwS8D97zx0Pc9U=
=TBJL
-----END PGP SIGNATURE-----
--0p6fkQojZ86Dx7oG--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120919061005.GR37286>