From owner-freebsd-arch@FreeBSD.ORG Thu Aug 14 16:07:20 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 53409414 for ; Thu, 14 Aug 2014 16:07:20 +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 258F429AA for ; Thu, 14 Aug 2014 16:07:20 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 05913B9C3; Thu, 14 Aug 2014 12:07:19 -0400 (EDT) From: John Baldwin To: Mateusz Guzik Subject: Re: current fd allocation idiom Date: Thu, 14 Aug 2014 11:44:50 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <20140717235538.GA15714@dft-labs.eu> <201408111124.52064.jhb@freebsd.org> <20140812233617.GA17869@dft-labs.eu> In-Reply-To: <20140812233617.GA17869@dft-labs.eu> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201408141144.51131.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 14 Aug 2014 12:07:19 -0400 (EDT) Cc: Konstantin Belousov , freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Aug 2014 16:07:20 -0000 On Tuesday, August 12, 2014 7:36:18 pm Mateusz Guzik wrote: > On Mon, Aug 11, 2014 at 11:24:51AM -0400, John Baldwin wrote: > > 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(). > > > > I would expect soabort to result in a timeout/reset as opposed to regular > connection close. > > Comments around soabort suggest it should not be used as a replacement > for close, but maybe this is largely because of what the other end will > see. That will need to be investigated. > > That said, I definitely support using delayed fd allocation (current > falloc_noinstall) where possible, but I'm not convinced it is safe for > all consumers. I think falloc_noinstall() failing is a rare case and is already going to cause application-level breakage that users have to handle anyway, so I'm not sure it's worth a lot of extra effort to support this edge case more gracefully. -- John Baldwin