Date: Thu, 14 Aug 2014 11:44:50 -0400 From: John Baldwin <jhb@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, freebsd-arch@freebsd.org Subject: Re: current fd allocation idiom Message-ID: <201408141144.51131.jhb@freebsd.org> In-Reply-To: <20140812233617.GA17869@dft-labs.eu> References: <20140717235538.GA15714@dft-labs.eu> <201408111124.52064.jhb@freebsd.org> <20140812233617.GA17869@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201408141144.51131.jhb>