isn't this addressed by the compiler builtins for atomics? -- richard
> On Oct 3, 2019, at 10:38 AM, Matthew Ahrens <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 > > <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-Mddd0b2abb7554ff63884d196> ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T3ee8a81d5f09f2ec-M745cf2e47158c481ffc95f8e Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
