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

126

151

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

335

Or make lock bool

336

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

123–128

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

134

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

151

Ok, I can eliminate the assert if you prefer.

335

Ok, bool it is.

336

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

134

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

126

Still not fixed

336

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

lib/libc/rpc/clnt_vc.c

134

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

156

[asomers]

lib/libc/rpc/clnt_vc.c

134

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

126

Still not fixed

lib/libc/rpc/clnt_vc.c

134

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

145

Why () are needed around mask?

624–625

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()?

803–804

Same notes there, about nesting.

[asomers]

lib/libc/rpc/clnt_dg.c

145

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

624–625

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

803–804

So why not use release_fd_lock() there?

lib/libc/rpc/clnt_vc.c

655–656

release_fd_lock()