Date: Fri, 22 Jan 2016 16:03:02 +0800 From: Marcelo Araujo <araujobsdport@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r294543 - head/usr.sbin/ypldap Message-ID: <CAOfEmZhZ=wU5Y4BqdvqhbyVKwSXZT-Gjw2Xcpby8xpMNGmWEog@mail.gmail.com> In-Reply-To: <20160122173028.H966@besplex.bde.org> References: <201601220302.u0M32dW2089530@repo.freebsd.org> <20160122173028.H966@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2016-01-22 15:57 GMT+08:00 Bruce Evans <brde@optusnet.com.au>:
> On Fri, 22 Jan 2016, Marcelo Araujo wrote:
>
> Log:
>> Switch from FD_SETSIZE to getdtablesize(2) as it can make the FD to be
>> tunable. Also it gets more close with the original implementation from
>> OpenBSD.
>>
>
> Modified: head/usr.sbin/ypldap/yp.c
>>
>> ==============================================================================
>> --- head/usr.sbin/ypldap/yp.c Fri Jan 22 02:28:17 2016 (r294542)
>> +++ head/usr.sbin/ypldap/yp.c Fri Jan 22 03:02:38 2016 (r294543)
>> @@ -83,17 +83,14 @@ void
>> yp_enable_events(void)
>> {
>> int i;
>> - extern fd_set svc_fdset;
>> struct yp_event *ye;
>>
>> - for (i = 0; i < FD_SETSIZE; i++) {
>> - if (FD_ISSET(i, &svc_fdset)) {
>> - if ((ye = calloc(1, sizeof(*ye))) == NULL)
>> - fatal(NULL);
>> - event_set(&ye->ye_event, i, EV_READ, yp_fd_event,
>> NULL);
>> - event_add(&ye->ye_event, NULL);
>> - TAILQ_INSERT_TAIL(&env->sc_yp->yd_events, ye,
>> ye_entry);
>> - }
>> + for (i = 0; i < getdtablesize(); i++) {
>> + if ((ye = calloc(1, sizeof(*ye))) == NULL)
>> + fatal(NULL);
>> + event_set(&ye->ye_event, i, EV_READ, yp_fd_event, NULL);
>> + event_add(&ye->ye_event, NULL);
>> + TAILQ_INSERT_TAIL(&env->sc_yp->yd_events, ye, ye_entry);
>> }
>> }
>>
>
> getdtablesize() is a syscall, so evaluating it every time in the loop is
> slow. Its value must be a loop invariant to work right. Compilers
> cannot reasonably tell that syscalls return invariant values. Maybe they
> should for the standard sysconf() values, but kernels have bugs like
> extra limits that break the invariants.
>
> Only old code or very unportable code should use getdtablize(). Code
> newer than 1990 should use {OPEN_MAX}. sysconf(_SC_OPEN_MAX) is
> almost as easy to use as getdtablesize() provided you don't handle
> errors for either.
>
> Old code in /usr/src provides many examples of better use of
> getdtablesize().
> In fact, the second largest subcollection of examples was in old yp code.
> In FreeBSD-~5.2:
>
> - login/login.c: this is the simplest good use. It uses a count-down loop
> so that the top limit is only evaluated once
>
> - rpcgen/rpc_svcount.c: this is careful to generate good code. It
> generates
> a loop like the above, but with the loop invariant moved out of the loop.
>
> - window/wwinit.c: this assigns getdtablesize() to a global but then never
> uses the value. This was even more broken in 4.4BSD-Lite2.
> getdtablesize() was unused but the comment on the variable said that it
> was used. The variable was the value of the highest fd seen, and was
> unused.
>
> This looks like some floundering to avoid using very large values
> of the limit. It is never right to just use the result of the
> syscall, or {OPEN_MAX}. E.g., blindly closing {OPEN_MAX} fd's in
> daemons is bad since it might make them take hours to start up.
> {OPEN_MAX} is 706977 on freefall.
>
> - ppp/bundle.c, ppp/chap.c, ppp/chat.c, ppp/command.c, ppp/exec.c: top-down
> loop
> - ppp/defs.c: used with malloc(). With no error checking of course.
>
> - rpc/yppasswdd/yppasswdd_main.c, rpc/ypupdate/ypupdate_main.c,
> rpc/ypxfrd/ypxfrd_main.c, ypserv/yp_main.c: bottom-up loop with
> invariant evaluated earlier
>
> - slip/sliplogin.c bottom-up loop with invariant evaluated earlier
>
> - sysinstall/install.c, sysinstall/network.c, sysinstall/user.c: top-down
> loop
> - sysinstall/system.c: 1 bottom-up-loop with invariant evaluated earlier,
> and 1 top-down loop
>
> - syslogd/syslogd.c: top-down loop
>
> - libexec/ftpd.c: home made popen() that mallocs() an array of size
> getdtablesize(). This actually has some error handling
>
> - rbootd/rbootd.c: bottom-up-loop with invariant evaluated earlier
>
> - rexecd/rexecd.c: top-down loop
>
> - rshd/rshd.c: top-down loop
>
> So all the examples were not too bad, modulo the bug that the API is
> broken as designed (it is OK if {OPEN_MAX} is 20 instead of 706977
> even on old systems where 20 is large).
>
> {OPEN_MAX} was surprisingly little used in *bin and libexec. I once
> tried to remove all instances of OPEN_MAX and almost succeeded
> (OPEN_MAX is the wrong static limit for {OPEN_MAX} and shouldn't
> exist). The only literal matches for OPEN_MAX were 4 in gperf and
> 4 in compat code in cron.
>
> Now, OPEN_MAX is sed just twice more in *bin and libexec. It is
> used in hastd as sysconf(_SC_OPEN_MAX) since hastd is too far from
> BSD style to use getdtablesie(). The style differences include
> too much error handling instead of too little.
>
> getdtablesize() is now used in much the same places as in ~5.2, except:
> - rexecd, slip_login, sysinstall and window went away
> - I missed uses in atm in ~5.2, and there are none there now
> - I missed a use in pkg_install in ~5.2, and it went away
> - a more careful grep somehow found an inconsistent set for yp*
> - large changes for syslogd and login. Without looking at the code,
> I think that these are to reduces the slowness of looking at
> thousands or millions of fd's. Looking at the code shows that
> both use closefrom(3). closefrom() was new in FreeBSD-8.
>
> The following old getdtablesize() loops can be converted to closefrom():
> - rbootd, rexecd, rshd, atmarpd, scspd, lpr (another one I missed which
> has already been converted), pkg_install, ppp (1 instance -- most
> loops are for fcntl), sliplogin, sysinstall (all 8 instances)
>
> closefrom() is now used in:
> - casperd, devd, hastd (no error handling instead of too much), login,
> jail, lpr, syslogd.
>
> Unfortunately, getdtablesize() is still needed for arrays as used in yp*,
> and for anything using select(). If getdtablesize() is large then the
> arrays should be sparse or large fd's should not be actually used.
> closefrom() should probably be used early to kill unknown fd's to make
> space for private fd's. After that, getdtablesize() becomes almost
> useless. You just allocate a table large enough for the fd's that
> you allocate, and don't allocate fd's sparsely.
>
> POSIX and C guarantee that a small number (about 8 or 20) can be opened.
> Hostile environments break even this. Just open all except 7 fd's
> and fork-exec or popen() your favourite application or C/POSIX test.
> stdio should probably close some fd's to implement the guarantee. But
> none of libc and very few applications use closefrom().
>
> Bruce
>
I noticed that getdtablesize(2) specifically for ypldap(8) case is a bit
slower than FD_SETSIZE, mostly because of the size of max_fd.
However, as ypldap(8) was imported from OpenBSD seemed good to keep it as
much as synced with OpenBSD implementation. Although I'm not sure if
FreeBSD and OpenBSD shares the same getdtablesize(2) implementation.
Not an excuse for sure, but I agree with you!!!
Best,
--
--
Marcelo Araujo (__)araujo@FreeBSD.org
\\\'',)http://www.FreeBSD.org <http://www.freebsd.org/>; \/ \ ^
Power To Server. .\. /_)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOfEmZhZ=wU5Y4BqdvqhbyVKwSXZT-Gjw2Xcpby8xpMNGmWEog>