It has been a while since I looked at this code, and I'd forgotten some of the convolution implicit in the "pluggability" of the ERR API. Something else for the TODO list I guess. I doubt that anyone is making use of that flexibility, and it would be massively simpler to carve it down to a single compile-time implementation. I think the same is true of the locking code for that matter, and DSO, and ...
First, you're right, pthreads_locking_callback() is collapsing everything to a mutex. If you have a patch for it to use pthread_wrlock_*() instead, I'd be interested to see it. Otherwise I could probably hack one together relatively quickly, the real challenge is subjecting it to a workload and getting results - let me know if you would prefer to test a patch I'd cooked up or to roll your own. BTW, I assume that typical pthread implementations, in the fast-path case (read-lock, no contention), require no context-switch? Second, I can't believe that checking for the presence of error state requires a lock, but from your description is sounds like this is the case. And as I dig it looks like this is indeed the case (but only requires a read-lock, hence the crux of your point). Are you able to confirm that crypto/err/err.c::int_thread_get_item() is the hot-spot for this excessive locking? Also, I assume that the workload where you see this is not getting actual errors, right? Ie. it's just frequently checking for errors and there's nothing there, I assume? If so, it seems to me that we should be able to make this nothing-to-report case lockless, at some marginal expense to the slow-path (when there is something to report). But if you tell me that the fast-path of a read-lock essentially does this already, then it would be of questionable value for me to add any avoidance logic to openssl itself. Cheers, Geoff On Fri, Jun 6, 2014 at 9:47 PM, Salz, Rich <rs...@akamai.com> wrote: > A colleague here noticed that the pthreads-based locking loses the > distinction between read and write locks. We’ve collected mutex contention > data, and found that the CRYPTO_ERR lock, used while getting error info, is > one of the biggest offenders. > > > > It turns out that pthreads_locking_callback ignores the CRYPTO_READ/WRITE > flag that is passed in. It seems fairly simple to update that function to > use NPTL rwlock’s. Any interest? We’ll put out a diff and pull request > soon. > > > > -- > > Principal Security Engineer > > Akamai Technologies, Cambridge, MA > > IM: rs...@jabber.me; Twitter: RichSalz > > >
