From patchwork Wed Nov 4 10:59:31 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 7548831 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DC9B39F327 for ; Wed, 4 Nov 2015 10:59:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BACF6206A3 for ; Wed, 4 Nov 2015 10:59:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 84BC220647 for ; Wed, 4 Nov 2015 10:59:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964918AbbKDK7g (ORCPT ); Wed, 4 Nov 2015 05:59:36 -0500 Received: from mail-lf0-f52.google.com ([209.85.215.52]:33778 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932467AbbKDK7e (ORCPT ); Wed, 4 Nov 2015 05:59:34 -0500 Received: by lfbf136 with SMTP id f136so37268823lfb.0 for ; Wed, 04 Nov 2015 02:59:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=from:to:cc:subject:organization:references:date:in-reply-to :message-id:user-agent:mime-version:content-type; bh=uxbUwSYr1WiMPnbHnS72FpckEzM6dbN10l6J25e1MlU=; b=UGpl/7TskCdbjHCmwPL/iTBvsFHNzQu6ngAyPv0hy/9HK/34zt6ua8m+vSGnGo3cYh 7IZMAkDvynL1/85cHdUOVAK2tpBiSpBoCXXFyedky5BRgBavNGXSpELDUyBSlO6vYx9+ jKNXVVos0ew0HLnO84c6GizEbsP/GDK2fbrZ0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:organization:references:date :in-reply-to:message-id:user-agent:mime-version:content-type; bh=uxbUwSYr1WiMPnbHnS72FpckEzM6dbN10l6J25e1MlU=; b=ApivKgf/c+cPoterm6AR+sLwZNF8B3HkHDFCDY554jSzX3Veqq0bQWjYNohWePQFI+ kbsKArWwcZScwDwperVZH/zMW2o6Ml0NL5/5gIaGGrmG1MgvDN8D2q321MHhTrGb3RoB +3xEHY0zoNJyx/nbhT8C5UB83e42JPSGC7QXxf+8IK9AsTfu+rpxK3W75Ulck7autAnE niKPJEbDPEB6pzwGHQ8fcY6R94KrOO06KU9Hdjq0hLZhQU5Fo8Fli8Y1F+bBN48lsGqN kj1oia/yskos5T0HOIArX+Sefr5+D4tcClLJio/H6ch9FOv9SqoTomVP5HMd0ntNAlTd MXXQ== X-Gm-Message-State: ALoCoQmgJAjuhxcGfMW03NPlCO+L2PdLJMBXx8fnhNoOxi2XxdYQt77l9t+muwKbKraWUeCgKIVs X-Received: by 10.25.15.42 with SMTP id e42mr8263lfi.49.1446634772905; Wed, 04 Nov 2015 02:59:32 -0800 (PST) Received: from morgan.rasmusvillemoes.dk ([130.225.20.51]) by smtp.gmail.com with ESMTPSA id ey10sm117298lbc.29.2015.11.04.02.59.31 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 04 Nov 2015 02:59:32 -0800 (PST) From: Rasmus Villemoes To: Eric Dumazet Cc: Alexander Viro , Linus Torvalds , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC] vfs: don't bother clearing close_on_exec bit for unused fds Organization: D03 References: <1446543679-28849-1-git-send-email-linux@rasmusvillemoes.dk> <1446600672.4184.1.camel@edumazet-glaptop2.roam.corp.google.com> X-Hashcash: 1:20:151104:eric.dumazet@gmail.com::5mLX7kRN5633N8Iw:0000000000000000000000000000000000000000V3A X-Hashcash: 1:20:151104:viro@zeniv.linux.org.uk::I2xH4P3SXpkSCuyK:000000000000000000000000000000000000002VFd X-Hashcash: 1:20:151104:torvalds@linux-foundation.org::uxFZqixWmoUEY4l5:000000000000000000000000000000007bhP X-Hashcash: 1:20:151104:linux-kernel@vger.kernel.org::0mlRz7c6Id30NzeI:000000000000000000000000000000000CBlH X-Hashcash: 1:20:151104:linux-fsdevel@vger.kernel.org::Ov1RcIwt9IU3iUp6:00000000000000000000000000000000IKRl Date: Wed, 04 Nov 2015 11:59:31 +0100 In-Reply-To: <1446600672.4184.1.camel@edumazet-glaptop2.roam.corp.google.com> (Eric Dumazet's message of "Tue, 03 Nov 2015 17:31:12 -0800") Message-ID: <87egg6c9nw.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Nov 04 2015, Eric Dumazet wrote: > On Tue, 2015-11-03 at 10:41 +0100, Rasmus Villemoes wrote: > >> @@ -667,7 +667,7 @@ void do_close_on_exec(struct files_struct *files) >> fdt = files_fdtable(files); >> if (fd >= fdt->max_fds) >> break; >> - set = fdt->close_on_exec[i]; >> + set = fdt->close_on_exec[i] & fdt->open_fds[i]; >> if (!set) >> continue; >> fdt->close_on_exec[i] = 0; > > If you don't bother, why leaving this final fdt->close_on_exec[i] = 0 ? Thanks, it should go, along with the mentioned memsets. Updated patch below. Reading dup_fd() I'm even more convinced that we're not relying on any particular value for close_on_exec bits for unused fds. After /* * The fd may be claimed in the fd bitmap but not yet * instantiated in the files array if a sibling thread * is partway through open(). So make sure that this * fd is available to the new process. */ we only __clear_open_fd(), so the close_on_exec bit may be left set in the new process. From: Rasmus Villemoes Date: Tue, 3 Nov 2015 09:43:53 +0100 Subject: [PATCH] vfs: don't bother clearing close_on_exec bit for unused fds In fc90888d07b8 (vfs: conditionally clear close-on-exec flag) a conditional was added to __clear_close_on_exec to avoid dirtying a cache line in the common case where the bit is already clear. However, AFAICT, we don't rely on the close_on_exec bit being clear for unused fds, except as an optimization in do_close_on_exec(); if I haven't missed anything, __{set,clear}_close_on_exec is always called when a new fd is allocated. At the expense of also reading through ->open_fds in do_close_on_exec(), we can avoid accessing the close_on_exec bitmap altogether in close(), which I think is a reasonable trade-off. The conditional added in the commit above still makes sense to avoid the dirtying on the allocation paths, but I also think it might make sense in __set_close_on_exec: I suppose any given app handling a non-trivial amount of fds uses O_CLOEXEC for either almost none or almost all of them, so after a while one would reach a sort of steady-state where bits in ->close_on_exec are almost never flipped. Signed-off-by: Rasmus Villemoes --- fs/file.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/fs/file.c b/fs/file.c index c6986dce0334..1bb74923395c 100644 --- a/fs/file.c +++ b/fs/file.c @@ -79,7 +79,6 @@ static void copy_fdtable(struct fdtable *nfdt, struct fdtable *ofdt) memcpy(nfdt->open_fds, ofdt->open_fds, cpy); memset((char *)(nfdt->open_fds) + cpy, 0, set); memcpy(nfdt->close_on_exec, ofdt->close_on_exec, cpy); - memset((char *)(nfdt->close_on_exec) + cpy, 0, set); cpy = BITBIT_SIZE(ofdt->max_fds); set = BITBIT_SIZE(nfdt->max_fds) - cpy; @@ -231,7 +230,8 @@ repeat: static inline void __set_close_on_exec(int fd, struct fdtable *fdt) { - __set_bit(fd, fdt->close_on_exec); + if (!test_bit(fd, fdt->close_on_exec)) + __set_bit(fd, fdt->close_on_exec); } static inline void __clear_close_on_exec(int fd, struct fdtable *fdt) @@ -369,7 +369,6 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp) int start = open_files / BITS_PER_LONG; memset(&new_fdt->open_fds[start], 0, left); - memset(&new_fdt->close_on_exec[start], 0, left); } rcu_assign_pointer(newf->fdt, new_fdt); @@ -644,7 +643,6 @@ int __close_fd(struct files_struct *files, unsigned fd) if (!file) goto out_unlock; rcu_assign_pointer(fdt->fd[fd], NULL); - __clear_close_on_exec(fd, fdt); __put_unused_fd(files, fd); spin_unlock(&files->file_lock); return filp_close(file, files); @@ -667,10 +665,9 @@ void do_close_on_exec(struct files_struct *files) fdt = files_fdtable(files); if (fd >= fdt->max_fds) break; - set = fdt->close_on_exec[i]; + set = fdt->close_on_exec[i] & fdt->open_fds[i]; if (!set) continue; - fdt->close_on_exec[i] = 0; for ( ; set ; fd++, set >>= 1) { struct file *file; if (!(set & 1))