This is an archive of the discontinued LLVM Phabricator instance.

[scudo] RFC thread specific data refactoring
AbandonedPublic

Authored by cryptoad on Sep 7 2017, 1:27 PM.

Details

Summary

This is a request for comment, not an actual patch submission.

I would like to get some reviewers feedback on a refactoring of how the thread
specific data is handled in Scudo. The Linux or Android distinction is too
narrow and can't be sensically extanded to other platforms.

The core of the idea is to not make this distinction anymore, but to introduce
a shared TSD model (N caches shared between threads, locking) vs an exclusive
TSD model (1 cache per thread, no locking required). This would allow for easy
platform inclusions, as demonstrated with the addition of Fuchsia here. The
model could ultimately be specified via defines as opposed to be set in stone
for a given platform (eg: we could do shared caches on Linux).

While the code included works, it's merely for demonstration purposes. The
previous organization involved .inc files per platform, which I am not opposed
to do again but felt cumbersome to deal with. I am looking for comments,
suggestions, ideas regarding code organization, naming, so that this refactoring
would make sense to others.

Thanks in advance!

Event Timeline

cryptoad created this revision.Sep 7 2017, 1:27 PM
eugenis added inline comments.Sep 7 2017, 5:02 PM
lib/scudo/scudo_allocator.cpp
460

Perhaps this could be done in a RAII fashion? Having to do unlock() on every successfull TSD access is error-prone.

vitalybuka added inline comments.Sep 7 2017, 5:05 PM
lib/scudo/scudo_allocator.cpp
460

e.g. with __sanitizer::at_scope_exit

dvyukov edited edge metadata.Sep 12 2017, 1:26 AM

Looks reasonable.

Would be easier to understand if s/ScudoThreadContext/ScudoTSD/ renaming is separated.
So this is basically no functional changes, just rebranding what we had for linux as "context per thread" and what we had for android as "several shared contexts", right?

The model could ultimately be specified via defines as opposed to be set in stone for a given platform (eg: we could do shared caches on Linux).

It would be interesting to benchmark Android with cache per thread model as well.

dvyukov added inline comments.Sep 12 2017, 1:34 AM
lib/scudo/scudo_allocator.cpp
255

This and getPrng look unnecessary now.

385

It feels that in this new model getTSDAndLock() should just never return NULL. If some TSD implementation can't return a thread-local descriptor, it should be _its_ problem to allocate and return a fallback descriptor instead. Since we now have a notion of getTSDAndLock()/TSD->unlock(), global mutex-protected fallback descriptor fits into into this model well.
But the "global hashed set of TSDs" implementation just does not have this problem.

lib/scudo/scudo_tsd.h
27

I think this file and the cc will be much easier to understand if split into multiple files (one per model).

So this is basically no functional changes, just rebranding what we had for linux as "context per thread" and what we had for android as "several shared contexts", right?

This is correct. Rebranding and reorganization of the code.

lib/scudo/scudo_allocator.cpp
255

I think I had them for accessor consistency more than anything else.

385

I think I tried that at some point.
I got into some issues with the fact that the fallback TSD is a shared one (eg: with mutex) while using exclusive per-thread TSDs (no locks).
This ended up with both TSD versions having to be present in the code (current it's one or the other) and added a layer of complexity that ended up being counter intuitive to me.
There is likely some more clever way to do it that I haven't thought about though. I'll have another look.

lib/scudo/scudo_tsd.h
27

I think I will indeed have to keep the previous multilple .h/.inc/.cpp organization.

It would be interesting to benchmark Android with cache per thread model as well.

As of now, Android still doesn't have ELF TLS but is using emutls for its thread_local variables, which doesn't work for us.

It would be interesting to benchmark Android with cache per thread model as well.

As of now, Android still doesn't have ELF TLS but is using emutls for its thread_local variables, which doesn't work for us.

We do have a TLS slot, check out get_android_tls_ptr.

We do have a TLS slot, check out get_android_tls_ptr.

My bad, misunderstood your comment.

cryptoad abandoned this revision.Sep 18 2017, 8:43 PM

Abandoning this, I am gonna work on an actual CL now.

lib/scudo/scudo_tls_android.inc