From owner-freebsd-arch@FreeBSD.ORG Mon Aug 11 21:28:53 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 71F48262 for ; Mon, 11 Aug 2014 21:28:53 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 48E162705 for ; Mon, 11 Aug 2014 21:28:53 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id C1A55B924; Mon, 11 Aug 2014 17:28:51 -0400 (EDT) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: current fd allocation idiom Date: Mon, 11 Aug 2014 11:24:51 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <20140717235538.GA15714@dft-labs.eu> <20140718155959.GN93733@kib.kiev.ua> <20140718191928.GB7179@dft-labs.eu> In-Reply-To: <20140718191928.GB7179@dft-labs.eu> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201408111124.52064.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 11 Aug 2014 17:28:51 -0400 (EDT) Cc: Konstantin Belousov , Mateusz Guzik X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Aug 2014 21:28:53 -0000 On Friday, July 18, 2014 3:19:28 pm Mateusz Guzik wrote: > On Fri, Jul 18, 2014 at 06:59:59PM +0300, Konstantin Belousov wrote: > > On Fri, Jul 18, 2014 at 04:40:12PM +0200, Mateusz Guzik wrote: > > > On Fri, Jul 18, 2014 at 04:06:29PM +0300, Konstantin Belousov wrote: > > > > It seems that all what is needed is conversion of places using > > > > falloc() to falloc_noinstall()/finstall(). > > > > > > > > > > This postpones fd allocation to after interested function did all work > > > it wanted to do, which means we would need reliable ways of reverting > > > all the work in case allocation failed. I'm not so confident we can do > > > that for all current consumers and both current and my proposed approach > > > don't impose such requirement. > > Cleanup should be identical to the actions done on close(2). > > > > > > > > Of course postponing fd allocation where possible is definitely worth > > > doing. > > Yes, and after that the rest of the cases should be evaluated. > > But my gut feeling is that everything would be converted. > > > > So let's say you accept() a connection. > > With current code, if you got to accept the connection you got it. > > With your proposal you may find that you can't allocate any fd and have > to close fp. This will be visible as accept + close by the other end, > while the caller never saw the connection. > > My guess is people would complain once they encounter such issue. Can't you already get this if you overflow the listen queue? (Having "accepted" connections aborted where the user application doesn't know): } else { /* * Keep removing sockets from the head until there's room for * us to insert on the tail. In pre-locking revisions, this * was a simple if(), but as we could be racing with other * threads and soabort() requires dropping locks, we must * loop waiting for the condition to be true. */ while (head->so_incqlen > head->so_qlimit) { struct socket *sp; sp = TAILQ_FIRST(&head->so_incomp); TAILQ_REMOVE(&head->so_incomp, sp, so_list); head->so_incqlen--; sp->so_qstate &= ~SQ_INCOMP; sp->so_head = NULL; ACCEPT_UNLOCK(); soabort(sp); ACCEPT_LOCK(); } TAILQ_INSERT_TAIL(&head->so_incomp, so, so_list); so->so_qstate |= SQ_INCOMP; head->so_incqlen++; I think the simplest approach would be to first convert as many places as possible to use falloc_noinstall() / finstall(). If you end up with all of them converted then you can just rename falloc_noinstall to falloc() and retire the old falloc(). -- John Baldwin