Re: pcp updates: libpcp multithreading - documentation and tools for call graphs & lock analysis

G'day Frank.

On 17/12/16 00:04, Frank Ch. Eigler wrote:
> ...
> It's only elapsed time that's improved with threading.  And latency -
> elapsed time - is our serious enemy in PCP.

The point I was trying make was the historical context in which the 
focus on single-threaded PCP code was the most important thing.  I 
remain unconvinced that multi-threading will help pmcd in any 
substantial way.  The latency issue for PMAPI clients is a relatively 
recent development in the evolution of PCP, and I do accept that for 
some environments this is problem area.

But we need to make libpcp thread-safe first (and we agree it is not 
today, especially for the use cases where there are lots of PMAPI 
contexts and these are being created, destroyed and reconnected a lot).

I view improved concurrency as an important, but secondary goal in this 
work.

>
> Page 2, regarding the Big Lock.  I believe that we are covering far
> too many individual critical sections with a single lock.  Many of the
> data structures that it protects could be handled with much smaller
> lifespan local locks.  Other than the lock-order-inversion problems,
> at the least the Big Lock impedes concurrency.

Until the code is cleaned up, I am reserving judgment here.  Because the 
structure of the calls is so complex, more local locks in code that 
calls back into other parts of libpcp seriously increases the chance of 
deadlock, which is worse than sub-optimal concurrency.

One place where we can make an immediate improvement is where we call 
external (usually glibc) routines that are not thread-safe ... we can 
look to rewrite these using thread-safe equivalents if possible, else 
add a new (non-recursive) lock that is held for the duration of these 
calls and is released before any call to another libpcp routine ... this 
will be entirely deadlock free.

> Page 3, regarding __pmLock_libpcp.  I believe that making this lock
> recursive was both an unfortunate and unnecessary design choice. ...

Unfortunate, agreed.

Unnecessary, I disagree.  The ABI and API compatibility requirements and 
the deep nesting of the code make it unclear (to me) that short of 
starting again with a green fields implementation this was or is an option.

>
> Regarding the unnecessary side, ... For example,
> one simple scheme have the pm* functions take any locks they require,
> then call __pm* siblings.  Forbid __pm* ones from calling locking pm*
> ones.

In the current library, especially with the spaghetti call structure 
that derived metrics have made significantly worse and all of the 
one-trip initialization blocks for locks, derived metrics configs, 
pcp.conf configs, ..., I think we might end up needing nearly _all_ 
locks before calling _any_ of the __pm* siblings.

Anyway, this is an implementation discussion that's ahead of where my 
thinking is at just at the moment.

> Regarding the __pmHandleToPtr c_lock vs. libpcp lock comments on page
> three, hallelujah, I'm glad we agree that this lookup race needs to be
> closed.

I don't believe there has been disagreement on the problem, but finding 
an acceptable solution has proven a challenge.

> Regarding the IPC-with-PMCD serialization, I still believe that
> dropping the socket sharing between multiple contexts to the same PMCD
> is the most robust and also easiest way to handle this whole area. ...

You may be right, but again I'm not ready to attack that yet.

> Regarding PM_CONTEXT_LOCAL on page 4, this is not too serious, but I
> am pretty sure we could permit multithreaded apps to call into the
> PMAPI even with such contexts. ...

This definitely sounds plausible to implement, but I question if there 
is a non-empty class of PMAPI clients that want to use PM_CONTEXT_LOCAL 
and have a real reason to be multi-threaded.  I don't a priori see a use 
case to drive this.

> The __pmAcc* stuff could use a rework to avoid global variables.
> Really, we need ACLs to be contextual rather than global anyway, such
> as for pmwebd inbound & outbound access controls.


Fair point, but outside the scope of my current work.

> Regarding derived metrics on page 5, ditto. ...  __dminit etc., and that
> does not need to lock the definitions - just work on a snapshot copy.

The problem is that the "snapshot" is not taken, rather the global 
expression trees are used to build the per-context expression trees and 
lots of PMAPI calls are made during this process, and the global trees 
need protection against any concurrent pmRegisterDerived() et al call. 
My plan here is to change __dminit to do just as you suggest and have a 
two phase process, namely lock, snapshot, unlock, populate the copied 
trees using PMAPI calls.

The document has not evolved to the point where that level of detail has 
been spelled out.

Thanks for the feedback.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#14926): https://groups.io/g/pcp/message/14926
View All Messages In Topic (3): https://groups.io/g/pcp/topic/3819767
Mute This Topic: https://groups.io/mt/3819767?uid=174580
New Topic: https://groups.io/g/pcp/post

Change Your Subscription: https://groups.io/g/pcp/editsub?uid=174580

Group Home: https://groups.io/g/pcp
Contact Group Owner: [email protected]

Terms of Service: https://groups.io/static/tos

Unsubscribe: https://groups.io/g/pcp/leave/354243/563757577/xyzzy

-=-=-
pcp mailing list
[email protected]
https://groups.io/g/pcp/messages
-=-=-=-=-=-=-=-=-=-=-=-