First, a note that to my shame I discovered Linux interface for atomic
operations only yesterday. And I must say that the proposed zatomic interface
looks very similar to what Linux has. I guess that this is not surprising.
Linux defines these types:
typedef struct {
int counter;
} atomic_t;
#ifdef CONFIG_64BIT
typedef struct {
long counter;
} atomic64_t;
#endif
And atomic functions operate on these types.
I got an impression that Linux does not provide atomic64_t operations in 32-bit
kernels. But I got a bit lost in the maze of indirections, so maybe someone
more familiar with Linux could clarify on this.
It is a little bit tempting to propose to switch OpenZFS to Linux atomics and
have other platforms provide compatibility shims for them.
The rest of the reply is inline below:
On 03/10/2019 20:38, Matthew Ahrens wrote:
> On Wed, Oct 2, 2019 at 7:51 AM Andriy Gapon > 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(>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.
After getting my feet wet in FreeBSD multi-platform support for atomics, I am
not sure about this myself.
Some 32-bit platforms provide adequate 64-bit atomics including load and store
operations. So, checking for just __LP64__ is too naive.
I guess that the check will have to be OS-specific
In fact, on FreeBSD if you have other 64-bit atomic operations, then you will
have load and store for sure.
> 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...
>
