Re: [Libtirpc-devel] [RFC PATCH 1/1] Support architectures with limited atomic support

[Chuck L. <chu...@gm...>]

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