libc/libc/rpc: refactor some global variables

[asomers]

Summary

• Combine dg_fd_locks and dg_cv into one array.
• Similarly for vc_fd_locks and vc_cv
• Turn some macros into inline functions

This is a mostly cosmetic change to make refactoring these strutures in
a future commit easier.

MFC after: 2 weeks
Sponsored by: Axcient

Switch the per-fd structs in clnt_{dg,vc}.c to RB Trees

This saves oodles of memory, especially when "ulimit -n" is large. It
also prevents a buffer overflow if getrlimit should fail.

PR: 274968
MFC after: 2 weeks
Sponsored by: Axcient

[kib]

lib/libc/rpc/clnt_dg.c

128

153

This assert is not too useful, the next line traps on NULL elem.

356

Or make lock bool

358

I do not think this is valid optimization. If the process is single-threaded, it cannot contend. But if between lock and unlock, process changes from single- to multi-threaded mode, you allow for the unlocked accesses.

lib/libc/rpc/clnt_vc.c

124

Condvars are not protected by mutex, they are used together with it.

135

Why is it useful to have per-client cv? You use global mutex, and it would be congested in the same way as global cv, one for all clients.

[asomers]

lib/libc/rpc/clnt_dg.c

153

Ok, I can eliminate the assert if you prefer.

356

Ok, bool it is.

358

That makes sense. But it's unrelated to the memory savings I'm trying to achieve. Would you accept this review as-is, and I make a followup to remove the single-threaded optimization?

lib/libc/rpc/clnt_vc.c

135

I don't know, but it's always been that way, apparently since at least 1999. The tirpc implementation used on GNU/Linux still allocates one condvar per file descriptor. Solaris eliminated condvars entirely, switching to use an actual mutex for each file descriptor instead of the bool "lock" variable. That switch must've happened sometime before 2005.

[asomers]

• Respond to kib's comments.

[kib]

lib/libc/rpc/clnt_dg.c

128

Still not fixed

358

Sure, it is a different change. I would put it before memory optimization in the patch series.

lib/libc/rpc/clnt_vc.c

135

Should we do the same? This sound more straightforward and IMO simply cleaner.

157

[asomers]

lib/libc/rpc/clnt_vc.c

135

It's a more intrusive change than what I'd planned. How would you test it? The ATF tests for rpc aren't very comprehensive.

[kib]

lib/libc/rpc/clnt_dg.c

128

Still not fixed

lib/libc/rpc/clnt_vc.c

135

Isn't just calling client side rpc enough?

BTW, switching to per-fd mutex also fixes the __isthreaded bug.

[asomers]

• style change to dg_fd_find and vc_fd_find
• rpc: Replace per-fd condvars with mutexes in clnt_{vc,dg}.c

[kib]

lib/libc/rpc/clnt_dg.c

147

Why () are needed around mask?

648–649

With minor reordering, can you use release_fd_lock() there?
Also, is nesting of clnt_fd_lock -> elem->mtx is needed? Can you confine clnt_fd_lock to dg_fd_find()?

831–832

Same notes there, about nesting.

[asomers]

lib/libc/rpc/clnt_dg.c

147

Because this function used to be a macro. I'll remove them.

648–649

With minor reordering, can you use release_fd_lock() there?

Yes

Also, is nesting of clnt_fd_lock -> elem->mtx is needed? Can you confine clnt_fd_lock to dg_fd_find()?

I'm not sure. I started to do that, but the old code held the clnt_fd_lock until after modifying xdrs. I don't know if that is necessary or an accident, so I left it.

[asomers]

• style fixup to "libc/libc/rpc: refactor some global variables"
• fixup to "rpc: Replace per-fd condvars with mutexes in clnt_{vc,dg}.c"

[kib]

Two minor issues with possible use of release_fd_lock() IMO worth fixing.

lib/libc/rpc/clnt_dg.c

831–832

So why not use release_fd_lock() there?

lib/libc/rpc/clnt_vc.c

683–684

release_fd_lock()