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

Hi, Ken -

On Sun, Dec 18, 2016 at 07:14:01AM +1100, Ken McDonell wrote:
> [...]
> 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.  

I didn't mention a multithreaded-pmcd in this early context;
multithreaded pcp clients already exist that do plenty of work without
requiring a multithreaded pmcd.  They do that because they connect to
different pmcds and/or archive files.


> [...]
> 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.

Sure; I'm confident we can improve both fronts though.  And really, if
we don't consider concurrency important, then there'd be little point
having any sort of concurrency-control finer than a single Even Bigger
Lock.


> >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.

I would not propose "more local locks in code that calls back into
other parts".  Rather, would promote local locks that are very
short-lived, are not held across function call boundaries (especially
not into faraway parts of libpcp), thus would not contribute to
deadlock risk.  And valgrind/helgrind is competent at semi-static
analysis of lock ordering problems.


> One place where we can make an immediate improvement is where we
> call external (usually glibc) routines that are not thread-safe
> [...]

I didn't have the sense that we have any serious problems on this
front.


> >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.

The ABI requirements are not so severe, in that we don't actually make
the operation of the __pmLock_libpcp a visible part of the ABI.  For
some reason, we do shlib-export it (?!?!!), but don't say when/how it
can be used.  Only a qa testcase even mentions it outside libpcp.  The
PMAPI certainly does not include explicit lock operations by clients.
So ISTM neither ABI nor API precludes or mandates a particular fix
approach.
 
As for whether a "green field" reimplementation is a necessity -
maybe.  I always enjoy the challenge of gentle retrofits rather than
redo-from-scratch, but OTOH this sort of thing is hard to unscramble.
(This was also briefly discussed in that URL I gave.)  We could
consider the present implementation a learning opportunity and try
again, with a more robust design.

Tooling such as your callgraph graph widget can probably help make
this (recursive -> nonrecursive, smaller) flag day more bearable.


> >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.

There need not be many such locks though.  Maybe only one or two, like
the context c_lock.


> 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.

Two separate -local- solutions for this were prototyped back in the
fche/multithread* branches, namely introducing a nesting between the
context[] array lock and the __pmContext c_lock.  It's the fallout
from that nesting elsewhere (easier deadlock induction due to
recursive libpcp lock) that was the problem.


- FChE

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

You receive all messages sent to this group.

View/Reply Online (#14927): https://groups.io/g/pcp/message/14927
View All Messages In Topic (4): 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
-=-=-=-=-=-=-=-=-=-=-=-