Hi, Ken -
> Sorry Frank this is taking so long I'm kinda busy at the moment with
> lots of projects on the go.
No problem.
> >Not quite: Deadlock (lock ordering) bugs were found in the code even
> >before this race condition was corrected. It just seemed like a good
> >time to fix it.
>
> OK, I'd like to drill down on this. Do you recall what lock ordering
> bugs? Do you have the recipe or evidence from experiments that
> demonstrate this?
Well sure. I have mentioned "valgrind --tool=helgrind" several times
now. Each "lock ordering error" report is such a case. The report
text can look cryptic. For example for the specific test case in [1]:
[1] http://www.pcp.io/pipermail/pcp/2016-July/011203.html
The issue there e.g. is that pmDestroyContext line 1494 takes the
libpcp lock, THEN line 1507 takes the c_lock. (That's the same
race-free proposed model for the __pmContextToPtr.) But helgrind has
found another case already where, holding the c_lock, some code tried
to take the libpcp lock. (That part of the report is elided from that
email.) So all someone would have to do to trigger that specific
deadlock is to run the pmDestroyContext concurrently with that other
code path.
That might look theoretical. That might look like a buggy PMAPI
client program. But maybe it's just one that happens to be heavily
multithreaded. And it's not just theoretical. Deadlocks are still
intermittently seen.
The intermittent / difficult-to-reproduce nature of these deadlocks
caused by lock ordering violations makes it important (yet again) not
to trust passing results in QA too much. A clean helgrind report over
a high-stress multithreading test would be more reassuring, but even
that is no guarantee.
> I would like to understand if these are (a) flaws in the original
> design, (b) errors in the original implementation, or (c) problems
> introduced by changes since the original implementation, e.g.
> derived metrics and connection logic and fetchgroups and ...
BTW fetchgroups is entirely lock-agnostic. It doesn't do any PM*LOCKs
of its own, relying on the lower libpcp layers. It's an ironclad
alibi! (OK, I lied a little bit; there is one type of use, for
grabbing some timestamps out of an archive context, but that's safe &
never been implicated.)
Anyway it doesn't seem to matter so much how we got here. The
questions are:
- whether we fix the __pmContextToPtr race (IMO: yes)
- whether we accept helgrind lock ordering reports as bugs (IMO: yes)
- what the Proper lock ordering is for libpcp vs other locks (IMO: complicated)
- how do we get there (IMO: deprecate libpcp lock ASAP)
- how do we stay there (IMO: institutionalize helgrind and high-stress
multithreading in QA)
> There have been a lot of changes in libpcp since the time the locks were
> first introduced, but I think we should be able to avoid all deadlock
> situations related to lock ordering.
I don't know. The current code has had such problems going back as
far as I've tried. The code with the race-free __pmContextToPtr
variant has many more instances.
> >Validity in the sense of non-freedness is probably not enough. The
> >delete/create race was only one possibility. We'd also have to ensure
> >the absence of *other* data races [...]
>
> Sorry, I don't follow this. The c_lock protocol is simple enough and
> local locking in context.c would guard against any multiple assignment
> of context / c_lock.
Maybe ... the point is that there is a moment during the current
__pmContextToPtr function when the context structure has been
"chosen", but is completely unprotected by any locks. During that
time, any other operation that could conceivably modify that same
chosen context structure could race in there and cause havoc. One
would have to analyze a lot of code to be even a little bit sure that
it's safe to stay lock-free during that moment.
> >>3. the array of pointers contexts[] (and the associated contexts_len)
> >>are local to context.c ... so we can implement a new and local lock to
> >>protect these [...]
> >
> >FWIW, this part is roughly what I started on the pcpfans.git
> >fche/multithread2 branch.
>
> OK, I is it worth trying to work through the issues that thwarted this
> approach. [...]
Yes, probably.
> >I'd do it with pthread_mutex_trylock. To assert non-lockedness,
> >assert that the trylock fails. To assert lockedness, assert that
> >trylock succeeds (and then release the lock).
> >
> >If we're to keep libpcp_lock recursive, then we won't be able to have
> >as strong assertions about non-lockedness as otherwise.
>
> For recursive locks, pthread_mutex_trylock() won't help establish if the
> lock is, or is not, already held.
Yes, that's what I meant.
> Getting rid of the recursive nature of __pmLock_libpcp is probably a
> goal for another (follow-on) project, as that will likely involve
> extensive re-factoring in the places where we call pm* routines
> recursively from within libpcp.
Yes, that's one way to pursue it, unless the current problems cannot
be solved without that follow-on project.
- FChE
|