Re: pcp updates: libpcp locking and concurrency control changes

G'day Frank, and thanks for the feedback.

On 16/01/17 11:49, Frank Ch. Eigler wrote:
> ...
> Nice.  Little nits, if I may?

Sure, nits (little or large) are most welcome.

> exit() probably doesn't need any /* THREADSAFE */ marking; it's pretty
> safe all right.  (We shouldn't call it much or at all from within the
> library, really.)

The THREADSAFE marking is really for the ../doc/find-unsafe script ... 
it means I've looked at this one, and determined that the use is 
threadsafe ... as you say the (a) any race on exit status really is not 
an issue, and (b) we should avoid exit() as much as possible ... I 
believe we really only go down an exit error path when all is lost, e.g. 
realloc() fails (or more generally OOM conditions), or when the PCP 
installation is borked, e.g. /etc/pcp.conf is missing, or some other 
world-ending scenario.

> ...
> opendir() etc. in our usage are probably safe, because we use it only
> on transient thread-local opendir dirp's.  No need for extcall locking
> there.

Yep, you're right ... I thought I'd removed the locking, but must have 
missed some cases ... I'll re-audit the opendir() blocks.

>>     [...] Additionally the lock ordering of the new local mutexes
>>     and __pmLock_extcall ensures these ones are deadlock free.
>
> Yeah, this is simple to show because "__pmLock_extcall" is a "leaf"
> lock, beneath which nothing else (in libpcp) is nested.  In
> pmns.c/tz.c, shouldn't there be locks around the getenv()'s?  Hm wait,
> _pushTZ's says "extcall" already acquired upon entry ... is that
> necessary?  Maybe we need an tz-stack specific lock.

I think it is necessary as these routines access and muck with the 
environment ... see next comment.

> deadlock-freeness (for this part) would be true even if you were to
> use different locks for the different "extcalls"; by using only a
> single one, you're imposing unnecessary serialization.  (e.g.,
> getenv() doesn't need one at all, except perhaps if you're racing
> against putenv(), and then an env-specific lock would be enough.  It
> certainly does not race with grent() etc.)

True.  But, I was hoping to address this use case scenario.

	Pamela the Punter is a PMAPI developer who wants to build a 
multi-threaded application that calls some POSIX unsafe routines outside 
the PMAPI use, e.g. getservbyport().  We can document, and Pamela can 
use a template like:
	PM_LOCK(__pmLock_extcall);
	getservbyport() code
	PM_UNLOCK(__pmLock_extcall);

This will work as long as everytime we call an external routine outside 
libpcp that is not thread safe we acquire (and later release) the 
__pmLock_extcall mutex.  And if Pamela does the same, we're all happy 
and safe.

If there is empirical evidence later that __pmLock_extcall is a 
bottleneck, I'll happily revist.  But a priori, the way this mutex is 
being used I would be most surprised if it is a measurable contributor 
to reduced concurrency ... we rarely call outside libpcp to 
thread-unsafe routines.

> Anyway, onto the big pay-off libpcp_lock stuff, yey.  You might
> consider pulling in pcpfans.git commit e99cd198f since it shows how to
> use valgrind/helgrind to do some basic run-time lock-correctness
> checking.  Other qa test cases could benefit from it.

Thanks for that ... I'm already playing with valgrind/helgrind but the 
code base is not in a fit state to commit QA tests yet ... but that will 
happen.


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

You receive all messages sent to this group.

View/Reply Online (#14978): https://groups.io/g/pcp/message/14978
View All Messages In Topic (3): https://groups.io/g/pcp/topic/4185439
Mute This Topic: https://groups.io/mt/4185439?uid=174580
New Topic: https://groups.io/g/pcp/post
-=-=-
pcp mailing list
[email protected]
https://groups.io/g/pcp/messages
-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-