Date: Fri, 16 Aug 2019 16:54:04 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Warner Losh <imp@bsdimp.com> Cc: Andriy Gapon <avg@freebsd.org>, "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org> Subject: Re: confused/concerned about ZFS / 64-bit atomics / 32-bit platforms Message-ID: <20190816135404.GB71821@kib.kiev.ua> In-Reply-To: <CANCZdfo8vm3hZxFFe2wvoLKSOp8K38kC%2BKHCVNhv-_Ct3LD0VQ@mail.gmail.com> References: <8bbee981-4f95-22eb-d9ec-00267c8e111d@FreeBSD.org> <20190816081406.GM2738@kib.kiev.ua> <CANCZdfo8vm3hZxFFe2wvoLKSOp8K38kC%2BKHCVNhv-_Ct3LD0VQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Aug 16, 2019 at 07:32:15AM -0600, Warner Losh wrote: > On Fri, Aug 16, 2019 at 2:14 AM Konstantin Belousov <kostikbel@gmail.com> > wrote: > > > On Fri, Aug 16, 2019 at 10:23:01AM +0300, Andriy Gapon wrote: > > > > > > I somewhat confused with respect to what guarantees C provides with > > > respect to accessing 64-bit objects on 32-bit platforms, if any. > > > I am also concerned about the use of 64-bit atomic values in ZFS given > > > that FreeBSD seems to support ZFS on 32-bit platforms (powerpc, i386, > > ...). > > > > > > My concerns stems from a failed import of a ZFS change from illumos. > > > That change has this pattern: > > > > > > volatile uint64_t *p; > > > uint64_t x, y; > > > ... > > > x = *p; > > > ... > > > atomic_foo_64(p, y); > > > > > > Specifically, I am concerned that there can be a torn read in x=*p > > > assignment on 32-bit platforms even if they provide a native > > > implementation of atomic_foo_64(). I am even more concerned about > > > platforms where atomic_foo_64() is not available and we need to emulate > > > it in opensolaris_atomic.c with the help from atomic_mtx. > > > > > > In more general terms, I am concerned about plain reads of 64-bit > > > variables that are manipulated atomically elsewhere, on 32-bit platforms. > > > Is my concern justified? > > Yes, your concerns are justified. Plain reads of 64bit variables on 32bit > > arches are not atomic. Also we do not provide e.g. atomic_load_64(9) on > > 32bit machines. > > > > Should we? I mean for those that support it? For i386 the implementation is trivial, I put the patch below, not tested. For arm, apparently there is atomic_load_64/atomic_store_64 already. > > > > On some architectures, there might be specific tricks to ensure > > atomicity of load and store for 64bit vars, e.g. on i386 (actually > > Pentium and newer) with use of of the CMPXCHG8 instruction, using it in > > dumb mode. ARM seems to provide LDREXD/STREXD. But AFAIK 32 ppc does not > > have any means to do it. > > > > My take: On those platforms that can do 64-bit atomics can support this > software, those that can't don't get support for this software. > > i386 and armv[67] are still big enough players that supporting ZFS on them > is desirable (though not mandatory). Since they have the functions, we > should enable building there and fix bugs that are identified. If there's > too many, or they are too elusive, we might want to disable by default > selectively. armv6, if it differs from armv7, is likely OK to mark ZFS as > broken there given how little power / memory the PiB and Pi0 have as well > as a lack of a good way to connect mass storage (just USB which exacerbates > the power situation on systems where power is already fragile). > > PowerPC 32-bit really isn't setup for ZFS either. The systems tend to have > less memory and don't tend to have a lot of disk. While it's nice to run > ZFS in a single disk setup, UFS is adequate for those situations. Don't > build file servers with FreeBSD 13 on this platform. We should mark it as > broken here. We already do this with several drivers. > > mips can do 64-bit atomics on 32-bit platforms, but it's ugly via disabling > interrupts (at least I think I committed those).... But even fewer people > have 32-bit mips boxes that want ZFS too... We don't build ZFS on mips > today, IIRC, so this is kinda moot. > > If ZFS needs this (now or in the future), and there's some atomics that are > possible, but not yet implemented, on these 32-bit platforms, we should > mark ZFS broken on those platforms and let their supporters fill in the > gaps (though it would be nice to ask for help before doing this if its i386 > or armv[67]). diff --git a/sys/i386/include/atomic.h b/sys/i386/include/atomic.h index 0d673af7358..ee2fa421ff8 100644 --- a/sys/i386/include/atomic.h +++ b/sys/i386/include/atomic.h @@ -891,6 +891,8 @@ u_long atomic_swap_long(volatile u_long *p, u_long v); #define atomic_add_rel_64 atomic_add_64 #define atomic_subtract_acq_64 atomic_subtract_64 #define atomic_subtract_rel_64 atomic_subtract_64 +#define atomic_load_64 atomic_load_acq_64 +#define atomic_store_64 atomic_store_rel_64 /* Operations on pointers. */ #define atomic_set_ptr(p, v) \
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190816135404.GB71821>