On 04/10/2019 02:49, Richard Elling wrote: > isn't this addressed by the compiler builtins for atomics?
'This' -- what specifically? I do not think that plain reads of 64-bit objects on 32-bit platforms can be addressed by anything until they cease being plain. >> On Oct 3, 2019, at 10:38 AM, Matthew Ahrens <mahr...@delphix.com >> <mailto:mahr...@delphix.com>> wrote: >> >> On Wed, Oct 2, 2019 at 7:51 AM Andriy Gapon <a...@freebsd.org >> <mailto:a...@freebsd.org>> wrote: >> >> >> This is a work in progress report for my work on safer use of atomics in >> ZFS >> that was discussed in the August developer meeting. >> >> I took the approach suggested at the meeting of creating a new type that >> is to >> be used for all objects that should be modified atomically. >> The type is trivial: >> typedef struct zatomic64 { >> volatile uint64_t val; >> } zatomic64_t; >> >> I also made a decision upfront to use a wrapper type only for 64-bit >> objects as >> 32-bit objects should be already safe on all supported platforms where a >> native >> integer size is either 32 or 64 bit. >> >> Then, as suggested, I wrote trivial wrappers for all 64-bit atomic >> operations >> that are used in ZFS code. For example: >> static inline void >> zatomic_add_64(zatomic64_t *a, uint64_t val) >> { >> atomic_add_64(&a->val, val); >> } >> >> A side note on naming. I simply prepended original function names with >> 'z'. >> Although names like zatomic64_add() could be prettier and more aligned >> with the >> type name. >> >> After that I added three new functions: >> - zatomic_load_64 -- safe (with respect to torn values) read of a 64-bit >> atomic >> - zatomic_store_64 -- safe write of a 64-bit atomic >> - zatomic_init_64 -- unsafe write of a 64-bit atomic >> >> The last function just sets zatomic64_t.val and it is supposed to be >> used when >> it is known that there are no concurrent accesses. It's pretty >> redundant the >> field can be set directly or the whole object can be initialized with x = >> { 0 }, >> but I decided to add it for symmetry. >> >> The first two functions are implemented like this: >> static inline uint64_t >> zatomic_load_64(zatomic64_t *a) >> { >> #ifdef __LP64__ >> return (a->val); >> #else >> return (zatomic_add_64_nv(a, 0)); >> #endif >> } >> >> static inline void >> zatomic_store_64(zatomic64_t *a, uint64_t val) >> { >> #ifdef __LP64__ >> a->val = val; >> #else >> (void)zatomic_swap_64(a, 0); >> #endif >> } >> >> I am not sure if this was a right way to do it. >> Maybe there should be a standard implementation only for 64-bit >> platforms and >> each 32-bit platform should do its own thing? >> For example, there is more than one way to get atomic 64-bit loads on >> x86, >> according to this: >> >> https://stackoverflow.com/questions/48046591/how-do-i-atomically-move-a-64bit-value-in-x86-asm >> >> >> What you did above seems fine to me. >> >> >> Anyway, I started converting ZFS (/FreeBSD) code to use the new type. >> Immediately, I got some problems and some questions. >> >> First, I am quite hesitant about what to do with kstat-s. >> I wanted to confine the change to ZFS, but kstat is an external thing (at >> least >> on illumos). For now I decided to leave the kstat-s alone. Especially >> as >> I was >> not going to change any code that reads the kstat-s. >> But this also means that some things like arc_c and arc_p, which are >> aliases to >> kstat value, remain potentially unsafe with respect to torn reads. >> >> >> Yeah, we would have to convert these kstats to have callbacks that do the >> atomic reads. >> >> >> >> I have not converted atomics in the aggsum code yet. >> I am actually a little bit confused about why as_lower_bound and >> as_upper_bound >> are manipulated with atomics. All manipulations seems to be done under >> as_lock. >> Maybe I overlooked something... >> >> >> The comment in aggsum_flush_bucket() has the (perhaps incorrect) >> justification: >> >> /* >> * We use atomic instructions for this because we read the upper and >> * lower bounds without the lock, so we need stores to be atomic. >> */ >> atomic_add_64((volatile uint64_t *)&as->as_lower_bound, asb->asc_delta); >> >> So the assumption is that as long as we do an atomic store, we can read with >> a >> normal read. The lock doesn't help because the readers don't hold the lock. >> >> >> >> I converted refcount.h. >> That had a bit of unexpected effect on rrwlock_t. >> zfs_refcount_t is used for rr_anon_rcount and rr_linked_rcount fields. >> The fields are always accessed rr_lock, so their atomicity is not >> actually >> required. I guess that zfs_refcount_t is used for the support of >> references >> with ownership. >> >> >> That's right. >> >> >> So, some bits of the rrwlock code access those fields through >> the refcount interface, but other (conditional) pieces directly access >> rc_count >> as a simple integer. >> So, there are a couple of options here. The direct accesses can be >> replaced >> with refcount accesses, but then we get unnecessary atomic operations in >> the >> fast paths. >> >> >> That would be true only on 32-bit platforms, right? I don't think a small >> performance hit to 32-bit kernels is worth worrying about. >> >> >> Or the code can keep directly accessing internals of both >> zfs_refcount_t and zatomic64_t. Another option is to create a copy of >> zfs_refcount_t that relies on external synchronization (so that it does >> not use >> atomics or any internal locks). But that would be a bit of code >> duplication. >> >> >> I'd prefer to keep the code simple, consistent, and easy to see its >> correctness, even if it costs some performance on 32-bit. >> >> >> >> Yet another hard case is znode's z_size. >> It's changed with atomic_cas_64() in a single place, in all other places >> it's >> used as a normal integer. On the one hand, there can be a torn read >> because of >> that atomic operation. On the other hand, it's quite messy to update >> all uses >> of z_size. >> >> So, the work is progressing, but I feel like limiting the scope of the >> change. >> Originally I planned to convert everything to zatomic in one go. >> But now I feel that just a few things really need that. >> Probably os_obj_next_percpu. >> Likely zfs_refcount_t. >> Anything else? >> I am thinking that maybe a few things could be converted initially and >> the >> rest >> could be converted on as needed basis. >> >> I don't like having a mix of regular atomics and zatomics, but alas. >> >> Any suggestions, comments, observations, ideas, etc are very welcome! >> >> P.S. >> Another thought is creeping in: maybe it's not worth bothering with ZFS >> on >> 32-bit platforms. >> >> >> Fine by me, although I think that opinions may vary on this one :-) >> Especially as long as some of the most popular operating systems to run >> OpenZFS support 32-bit kernels (although it looks like FreeBSD and Ubuntu are >> in the process of dropping support for i386? But ZoL still supports much >> older releases of Ubuntu and RHEL.). >> >> --matt > > *openzfs <https://openzfs.topicbox.com/latest>* / openzfs-developer / see > discussions <https://openzfs.topicbox.com/groups/developer> + participants > <https://openzfs.topicbox.com/groups/developer/members> + delivery options > <https://openzfs.topicbox.com/groups/developer/subscription> Permalink > <https://openzfs.topicbox.com/groups/developer/T3ee8a81d5f09f2ec-M2869ca2fabcee5f4249ece1f> > -- Andriy Gapon ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T3ee8a81d5f09f2ec-Mbeda3898a9fc51e251f56538 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
