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

130

155

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

346–347

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.

347

Or make lock bool

lib/libc/rpc/clnt_vc.c

126–132

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

138

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

155

Ok, I can eliminate the assert if you prefer.

346–347

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?

347

Ok, bool it is.

lib/libc/rpc/clnt_vc.c

138

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

130

Still not fixed

346–347

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

lib/libc/rpc/clnt_vc.c

138

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

160

[asomers]

lib/libc/rpc/clnt_vc.c

138

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

130

Still not fixed

lib/libc/rpc/clnt_vc.c

138

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

149

Why () are needed around mask?

632–633

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

808–809

Same notes there, about nesting.

[asomers]

lib/libc/rpc/clnt_dg.c

149

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

632–633

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

808

So why not use release_fd_lock() there?

lib/libc/rpc/clnt_vc.c

660

release_fd_lock()