From owner-svn-src-all@freebsd.org Wed Oct 14 12:28:42 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 96930437DE5; Wed, 14 Oct 2020 12:28:42 +0000 (UTC) (envelope-from arichardson@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CBBXp3VpBz3drF; Wed, 14 Oct 2020 12:28:42 +0000 (UTC) (envelope-from arichardson@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 5781CE4B8; Wed, 14 Oct 2020 12:28:42 +0000 (UTC) (envelope-from arichardson@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 09ECSgL9023439; Wed, 14 Oct 2020 12:28:42 GMT (envelope-from arichardson@FreeBSD.org) Received: (from arichardson@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 09ECSg0D023438; Wed, 14 Oct 2020 12:28:42 GMT (envelope-from arichardson@FreeBSD.org) Message-Id: <202010141228.09ECSg0D023438@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: arichardson set sender to arichardson@FreeBSD.org using -f From: Alex Richardson Date: Wed, 14 Oct 2020 12:28:42 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r366697 - head/usr.bin/xinstall X-SVN-Group: head X-SVN-Commit-Author: arichardson X-SVN-Commit-Paths: head/usr.bin/xinstall X-SVN-Commit-Revision: 366697 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Oct 2020 12:28:42 -0000 Author: arichardson Date: Wed Oct 14 12:28:41 2020 New Revision: 366697 URL: https://svnweb.freebsd.org/changeset/base/366697 Log: install(1): Avoid unncessary fstatfs() calls and use mmap() based on size According to git blame the trymmap() function was added in 1996 to skip mmap() calls for NFS file systems. However, nowadays mmap() should be perfectly safe even on NFS. Importantly, onl ufs and cd9660 file systems were whitelisted so we don't use mmap() on ZFS. It also prevents the use of mmap() when bootstrapping from macOS/Linux since on those systems the trymmap() function was always returning zero due to the missing MFSNAMELEN define. This change keeps the trymmap() function but changes it to check whether using mmap() can reduce the number of system calls that are required. Using mmap() only reduces the number of system calls if we need multiple read() syscalls, i.e. if the file size is > MAXBSIZE. However, mmap() is more expensive than read() so this sets the threshold at 4 fewer syscalls. Additionally, for larger file size mmap() can significantly increase the number of page faults, so avoid it in that case. It's unclear whether using mmap() is ever faster than a read with an appropriate buffer size, but this change at least removes two unnecessary system calls for every file that is installed. Reviewed By: markj Differential Revision: https://reviews.freebsd.org/D26041 Modified: head/usr.bin/xinstall/xinstall.c Modified: head/usr.bin/xinstall/xinstall.c ============================================================================== --- head/usr.bin/xinstall/xinstall.c Wed Oct 14 10:12:39 2020 (r366696) +++ head/usr.bin/xinstall/xinstall.c Wed Oct 14 12:28:41 2020 (r366697) @@ -148,7 +148,7 @@ static void metadata_log(const char *, const char *, s const char *, const char *, off_t); static int parseid(const char *, id_t *); static int strip(const char *, int, const char *, char **); -static int trymmap(int); +static int trymmap(size_t); static void usage(void); int @@ -1087,7 +1087,7 @@ compare(int from_fd, const char *from_name __unused, s if (do_digest) digest_init(&ctx); done_compare = 0; - if (trymmap(from_fd) && trymmap(to_fd)) { + if (trymmap(from_len) && trymmap(to_len)) { p = mmap(NULL, from_len, PROT_READ, MAP_SHARED, from_fd, (off_t)0); if (p == MAP_FAILED) @@ -1248,13 +1248,8 @@ copy(int from_fd, const char *from_name, int to_fd, co digest_init(&ctx); - /* - * Mmap and write if less than 8M (the limit is so we don't totally - * trash memory on big files. This is really a minor hack, but it - * wins some CPU back. - */ done_copy = 0; - if (size <= 8 * 1048576 && trymmap(from_fd) && + if (trymmap((size_t)size) && (p = mmap(NULL, (size_t)size, PROT_READ, MAP_SHARED, from_fd, (off_t)0)) != MAP_FAILED) { nw = write(to_fd, p, size); @@ -1523,20 +1518,23 @@ usage(void) * return true (1) if mmap should be tried, false (0) if not. */ static int -trymmap(int fd) +trymmap(size_t filesize) { -/* - * The ifdef is for bootstrapping - f_fstypename doesn't exist in - * pre-Lite2-merge systems. - */ -#ifdef MFSNAMELEN - struct statfs stfs; - - if (fstatfs(fd, &stfs) != 0) - return (0); - if (strcmp(stfs.f_fstypename, "ufs") == 0 || - strcmp(stfs.f_fstypename, "cd9660") == 0) - return (1); -#endif - return (0); + /* + * This function existed to skip mmap() for NFS file systems whereas + * nowadays mmap() should be perfectly safe. Nevertheless, using mmap() + * only reduces the number of system calls if we need multiple read() + * syscalls, i.e. if the file size is > MAXBSIZE. However, mmap() is + * more expensive than read() so set the threshold at 4 fewer syscalls. + * Additionally, for larger file size mmap() can significantly increase + * the number of page faults, so avoid it in that case. + * + * Note: the 8MB limit is not based on any meaningful benchmarking + * results, it is simply reusing the same value that was used before + * and also matches bin/cp. + * + * XXX: Maybe we shouldn't bother with mmap() at all, since we use + * MAXBSIZE the syscall overhead of read() shouldn't be too high? + */ + return (filesize > 4 * MAXBSIZE && filesize < 8 * 1024 * 1024); }