|
From: Chuck L. <chu...@gm...> - 2015-08-13 16:35:38
|
On Thu, Aug 13, 2015 at 7:39 AM, Brendan Heading <bre...@gm...> wrote: >> These really need to be kept inside the library. Library consumers will not >> be guaranteed to have defined HAVE_SYNC_BUILTIN. >> >> IMO there's no need for these to be inline. I doubt they are even part of >> the library API, but I need to review headers in other TI-RPC implementations. >> >> I'm traveling this week, apologies for not responding before. I will try to >> respond further later today when I have a real keyboard. :-) > > Chuck, > > I'll have another look at it. I've approached it so far on the basis > of "minimal changes which replicate the existing functionality" but it > sounds like I'll need to dive a bit deeper. > > FWIW all of this is about performance, and it makes sense as the > __sync_fetch_and_add stuff is typically a very small additional > overhead over and above the increment operation. If it becomes a > function call, the benefit of doing it using atomics instead of a > mutex is essentially lost. > > If this code is not on a performance critical path I think that > inlining and atomics are certainly a premature optimization. The > original coder didn't think so though. The original coder didn't mention a performance consideration, and did not provide data that demonstrated a problem. There is no rationale provided for the use of GCC-specific compiler features (including the use of a variadic macro in a public header, which had to be fixed later; see commits f8104ba924be and 9ce88a378679). The purpose of this library is to provide generic RPC functionality in user space which is compatible with glibc's and Solaris's user space RPC implementation. If performance is needed, then a specialty library should be used. As an example, I offer Matt Benjamin's RPC library used by NFS/Ganesha. So, IMO there is no performance requirement here. The use of __sync_fetch_and_add is not warranted and it adds a hard dependence on GCC, which is not desirable and should be replaced, as you are attempting to do now. I think the larger consideration in this case is compatibility with other TI-RPC libraries. The original coder broke this by adding extra processing during AUTH_DESTROY. I don't see auth_get() or auth_put() in glibc's headers, and the AUTH_DESTROY there is simple. Likewise in Solaris's /usr/include/rpc/auth.h. Therefore I think a correct solution would return libtirpc to the original API, and handle critical section serialization inside the library where it can't be screwed up by applications. The synchronization primitive should reside in the private structure pointed to by auth::ah_private. (Conditional compilation can be used there if needed.) Check if glibc already does this, and start with the same logic if possible for your fix. If not, the usual pattern is to add a global mutex in src/mt_misc.c for serializing auth reference count changes. The downside of fixing this correctly is that possibly a soname update would be required due to reverting the AUTH_DESTROY() changes. -- "We cannot take our next breath without the exhale." -- Ellen Scott Grable |