Page MenuHomeFreeBSD

Implement getdtablecount() to count open file descriptors.
ClosedPublic

Authored by rodrigc on Nov 5 2015, 1:49 AM.

Details

Summary

OpenBSD has this function implemented as a system call.
This implements getdtablecount() in terms of kinfo_getfile(3)
in libutil.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rodrigc retitled this revision from to Implement getdtablecount() to count open file descriptors..Nov 5 2015, 1:49 AM
rodrigc updated this object.
rodrigc edited the test plan for this revision. (Show Details)
rodrigc added reviewers: bapt, mjg.
rodrigc updated this revision to Diff 9953.
mjg edited edge metadata.Nov 5 2015, 11:27 AM

Is not this going to be too slow for practical use? This will do plenty of completely unnecessary work, including path name resolving etc.

Quick look at opensmtpd shows they use it before accept() and during connection teardown.

As such, I would expect it t get dog slow when actual traffic comes.

There was an attempt to implement this as a syscall last year made by hardenedbsd people. I see they ended up implemented something as a sysctl handle which seems like a reasonable idea. The code is not suitable for stealing though - it does a weird loop over the bitmap inspecting fields separately instead of doing population count, the handler is not marked mpsafe (I guess an omission), the sysctl does not have a static number and has a rather weird name.

That said, putting something under kern.proc.* alongside current handlers should do the trick. I can do that later.

lib/libopenbsd/getdtablecount.c
36 ↗(On Diff #9953)

Should not this be in a separate header?

51 ↗(On Diff #9953)

Is not this going to be too slow for practical use? This will do plenty of completely unnecessary work, including path name resolving etc.

Quick look at opensmtpd shows they use it before accept() and during connection teardown.

As such, I would expect it t get dog slow when actual traffic comes.

rodrigc added inline comments.Nov 5 2015, 4:06 PM
lib/libopenbsd/getdtablecount.c
51 ↗(On Diff #9953)

I went down this path, because that is what @bapt suggested to implement. Let's see if @bapt has any feedback.

bapt added inline comments.Nov 5 2015, 5:23 PM
lib/libopenbsd/getdtablecount.c
51 ↗(On Diff #9953)

kinfo_getfile, is calling a sysctl, imho getdtablecount() should directly call the same sysctl, just to count

@bapt: if I call the same sysctl, I will pretty much be duplicating the code in kinfo_getfile(): https://reviews.freebsd.org/diffusion/S/browse/head/lib/libutil/kinfo_getfile.c;290404$13
I can do that. Are you OK with that approach?

mjg added inline comments.Nov 5 2015, 8:58 PM
lib/libopenbsd/getdtablecount.c
51 ↗(On Diff #9953)

All the hard work is done by the sysctl in question, kinfo_getfile plays little role here. Additional overhead resulting from said sysctl is prohitibtive.

I hacked up a separate sysctl: https://people.freebsd.org/~mjg/patches/kern_proc_nfds.diff

I'm too lazy to implement a nice userspace part.

Can be easily called with

int nfds;
 size_t len = sizeof(nfds);
 mib[0] = CTL_KERN;
 mib[1] = KERN_PROC;
 mib[2] = KERN_PROC_NFDS;
 mib[3] = 0;

 error = sysctl(mib, 4, &nfds, &len, NULL, 0);

i.e. the required 4th argument is 0.

rodrigc edited edge metadata.Nov 5 2015, 11:01 PM
rodrigc updated this revision to Diff 9976.

Implement getdtablecount() using sysctl().

jhb added a subscriber: jhb.Nov 6 2015, 1:25 AM

I agree with @mjg. Fetching kinfo_file is quite heavyweight. Having a new KERN_PROC_NFDS (or NOPENFILES or something) would be prudent.

@bapt are you OK with me implementing getdtablecount() in terms of @mjg's new sysctl?

bapt edited edge metadata.Nov 6 2015, 7:04 AM

Of course, that I why I reply to @mjg that I like his sysctl :)

lib/libopenbsd/getdtablecount.c
51 ↗(On Diff #9953)

I do like this new sysctl and it will be useful for other use cases

mjg added a comment.Nov 7 2015, 12:19 AM

I committed said sysctl in https://svnweb.freebsd.org/changeset/base/290473

Usage is shown in my previous comment.

rodrigc edited edge metadata.Nov 7 2015, 5:07 AM
rodrigc updated this revision to Diff 10007.

Implement getdtablecount() using new sysctl implemented by mjg.

rodrigc marked 5 inline comments as done.Nov 7 2015, 5:45 AM
rodrigc added inline comments.
rodrigc updated this revision to Diff 10008.
lib/libopenbsd/getdtablecount.c
37 ↗(On Diff #10008)

I will implement a separate header, I just want to get some agreement on the implementation
first to make sure I am going down the right path.

bapt added a comment.Nov 7 2015, 10:55 AM

I thnk you can cleanup a bit your headers but otherwise looks good

bapt edited edge metadata.Nov 7 2015, 10:55 AM
bapt accepted this revision.
This revision is now accepted and ready to land.Nov 7 2015, 10:55 AM

@bapt: what should I do about the prototype for getdtablecount()? On OpenBSD, they put the prototype in <unistd.h>.
I was thinking of creating a header file libopenbsd.h and making it private to this
library, and putting it in there. Do you think I should do that, or do you have a better suggestion?

bapt added a comment.Nov 7 2015, 10:49 PM

Depends what do you plan to use that for

is it something you want to publicly expose, in that case it should be added to libc? or is it something only for use in base in which case then you want it in a libopenbsd and yes you would need to have a dedicated header

@bapt: For now, I need to expose the prototype somehow so that I can use it inside imsg code inside libopenbsd.
I can make a header internal to libopenbsd, or stick: `extern int getdtablecount(void);` in the imsg code to get things to compile.
Adding it to libc is doable, but I want to be conservative about adding a new symbol to libc. I've never done that before, and
am not familiar with all the things that need to be done when adding new stuff to libc in terms of symbol versioning, version bump, ports exp-run, etc.

mjg added a comment.Nov 14 2015, 10:27 AM

There is already an example in lib/libnetbsd - create unistd.h in lib/libopenbsd (but i would prefer lib/libopenbsd/include), put the declaration in and #include_next <unistd.h>. see lib/libnetbsd/sys/time.h for an example.

This revision was automatically updated to reflect the committed changes.