From 39790bd3a15a8999fbff1a831d99299a9aa72272 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Thu, 21 Apr 2022 22:25:28 -0700 Subject: [PATCH] switch remaining sync/atomic to atomicbitops for 32 bit values PiperOrigin-RevId: 443571047 --- pkg/coverage/coverage.go | 3 ++ pkg/flipcall/BUILD | 1 + pkg/flipcall/ctrl_futex.go | 24 ++++----- pkg/flipcall/flipcall.go | 19 ++++--- pkg/flipcall/flipcall_unsafe.go | 9 ++-- pkg/flipcall/futex_linux.go | 7 ++- pkg/lisafs/node.go | 9 ++-- pkg/log/log.go | 13 +++-- pkg/p9/client_file.go | 66 ++++++++++++------------- pkg/p9/handlers.go | 7 ++- pkg/p9/path_tree.go | 3 +- pkg/p9/server.go | 22 ++++----- pkg/refs/refcounter.go | 12 ++--- pkg/sentry/contexttest/contexttest.go | 7 ++- pkg/sentry/fs/dirent.go | 14 +++--- pkg/sentry/fs/dirent_state.go | 3 +- pkg/sentry/fs/inotify.go | 8 +-- pkg/sentry/fs/inotify_watch.go | 9 ++-- pkg/sentry/mm/address_space.go | 21 ++++---- pkg/sentry/mm/lifecycle.go | 14 +++--- pkg/sentry/mm/mm.go | 15 ++---- pkg/sentry/mm/syscalls.go | 9 ++-- pkg/sentry/pgalloc/BUILD | 1 + pkg/sentry/pgalloc/save_restore.go | 8 +-- pkg/sentry/time/BUILD | 1 + pkg/sentry/vfs/dentry.go | 8 ++- pkg/sentry/vfs/file_description.go | 24 ++++----- pkg/sentry/vfs/inotify.go | 28 +++++------ pkg/sentry/vfs/mount.go | 5 +- pkg/sleep/BUILD | 1 + pkg/sleep/sleep_test.go | 11 +++-- pkg/sync/gate_test.go | 1 + pkg/sync/mutex_test.go | 1 + pkg/syncevent/BUILD | 1 + pkg/syncevent/syncevent_example_test.go | 9 ++-- pkg/syncevent/waiter_test.go | 8 +-- pkg/unet/BUILD | 1 + pkg/unet/unet.go | 25 +++++----- pkg/unet/unet_unsafe.go | 7 ++- pkg/waiter/BUILD | 2 + pkg/waiter/waiter_test.go | 11 +++-- runsc/fsgofer/BUILD | 1 + runsc/fsgofer/lisafs.go | 26 +++++----- tools/checklocks/BUILD | 1 + tools/checklocks/state.go | 14 +++--- tools/checklocks/test/test.go | 2 + tools/go_fieldenum/main.go | 33 +++++++------ 47 files changed, 264 insertions(+), 261 deletions(-) diff --git a/pkg/coverage/coverage.go b/pkg/coverage/coverage.go index 8b7e567ad..79238ad51 100644 --- a/pkg/coverage/coverage.go +++ b/pkg/coverage/coverage.go @@ -23,6 +23,9 @@ // coverage surface. This causes bazel to use the Go cover tool manually to // generate instrumented files. It injects a hook that registers all coverage // data with the coverdata package. +// +// Using coverdata.Counters requires sync/atomic integers. +// +checkalignedignore package coverage import ( diff --git a/pkg/flipcall/BUILD b/pkg/flipcall/BUILD index c810c7946..520e1539d 100644 --- a/pkg/flipcall/BUILD +++ b/pkg/flipcall/BUILD @@ -15,6 +15,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/abi/linux", + "//pkg/atomicbitops", "//pkg/log", "//pkg/memutil", "//pkg/sync", diff --git a/pkg/flipcall/ctrl_futex.go b/pkg/flipcall/ctrl_futex.go index 99410628f..e101dc1c5 100644 --- a/pkg/flipcall/ctrl_futex.go +++ b/pkg/flipcall/ctrl_futex.go @@ -21,13 +21,13 @@ import ( "encoding/json" "fmt" "math" - "sync/atomic" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/log" ) type endpointControlImpl struct { - state int32 + state atomicbitops.Int32 } // Bits in endpointControlImpl.state. @@ -54,7 +54,7 @@ func (ep *Endpoint) ctrlConnect() error { if err := json.NewEncoder(w).Encode(struct{}{}); err != nil { return fmt.Errorf("error writing connection request: %v", err) } - *ep.dataLen() = w.Len() + *ep.dataLen() = atomicbitops.FromUint32(w.Len()) // Exchange control with the server. if err := ep.futexSetPeerActive(); err != nil { @@ -69,7 +69,7 @@ func (ep *Endpoint) ctrlConnect() error { // Read the connection response. var resp struct{} - respLen := atomic.LoadUint32(ep.dataLen()) + respLen := ep.dataLen().Load() if respLen > ep.dataCap { return fmt.Errorf("invalid connection response length %d (maximum %d)", respLen, ep.dataCap) } @@ -92,7 +92,7 @@ func (ep *Endpoint) ctrlWaitFirst() error { } // Read the connection request. - reqLen := atomic.LoadUint32(ep.dataLen()) + reqLen := ep.dataLen().Load() if reqLen > ep.dataCap { return fmt.Errorf("invalid connection request length %d (maximum %d)", reqLen, ep.dataCap) } @@ -106,7 +106,7 @@ func (ep *Endpoint) ctrlWaitFirst() error { if err := json.NewEncoder(w).Encode(struct{}{}); err != nil { return fmt.Errorf("error writing connection response: %v", err) } - *ep.dataLen() = w.Len() + *ep.dataLen() = atomicbitops.FromUint32(w.Len()) // Return control to the client. raceBecomeInactive() @@ -147,11 +147,11 @@ func (ep *Endpoint) ctrlWakeLast() error { } func (ep *Endpoint) enterFutexWait() error { - switch eps := atomic.AddInt32(&ep.ctrl.state, epsBlocked); eps { + switch eps := ep.ctrl.state.Add(epsBlocked); eps { case epsBlocked: return nil case epsBlocked | epsShutdown: - atomic.AddInt32(&ep.ctrl.state, -epsBlocked) + ep.ctrl.state.Add(-epsBlocked) return ShutdownError{} default: // Most likely due to ep.enterFutexWait() being called concurrently @@ -161,7 +161,7 @@ func (ep *Endpoint) enterFutexWait() error { } func (ep *Endpoint) exitFutexWait() { - switch eps := atomic.AddInt32(&ep.ctrl.state, -epsBlocked); eps { + switch eps := ep.ctrl.state.Add(-epsBlocked); eps { case 0: return case epsShutdown: @@ -175,7 +175,7 @@ func (ep *Endpoint) exitFutexWait() { func (ep *Endpoint) ctrlShutdown() { // Set epsShutdown to ensure that future calls to ep.enterFutexWait() fail. - if atomic.AddInt32(&ep.ctrl.state, epsShutdown)&epsBlocked != 0 { + if ep.ctrl.state.Add(epsShutdown)&epsBlocked != 0 { // Wake the blocked thread. This must loop because it's possible that // FUTEX_WAKE occurs after the waiter sets epsBlocked, but before it // blocks in FUTEX_WAIT. @@ -187,7 +187,7 @@ func (ep *Endpoint) ctrlShutdown() { break } yieldThread() - if atomic.LoadInt32(&ep.ctrl.state)&epsBlocked == 0 { + if ep.ctrl.state.Load()&epsBlocked == 0 { break } } @@ -199,7 +199,7 @@ func (ep *Endpoint) ctrlShutdown() { } func (ep *Endpoint) shutdownConn() { - switch cs := atomic.SwapUint32(ep.connState(), csShutdown); cs { + switch cs := ep.connState().Swap(csShutdown); cs { case ep.activeState: if err := ep.futexWakeConnState(1); err != nil { log.Warningf("failed to FUTEX_WAKE peer Endpoint for shutdown: %v", err) diff --git a/pkg/flipcall/flipcall.go b/pkg/flipcall/flipcall.go index 88588ba0e..55787d572 100644 --- a/pkg/flipcall/flipcall.go +++ b/pkg/flipcall/flipcall.go @@ -19,9 +19,9 @@ package flipcall import ( "fmt" "math" - "sync/atomic" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/memutil" ) @@ -53,9 +53,8 @@ type Endpoint struct { inactiveState uint32 // shutdown is non-zero if Endpoint.Shutdown() has been called, or if the - // Endpoint has acknowledged shutdown initiated by the peer. shutdown is - // accessed using atomic memory operations. - shutdown uint32 + // Endpoint has acknowledged shutdown initiated by the peer. + shutdown atomicbitops.Uint32 ctrl endpointControlImpl } @@ -144,7 +143,7 @@ func (ep *Endpoint) unmapPacket() { // Shutdown is the only Endpoint method that may be called concurrently with // other methods on the same Endpoint. func (ep *Endpoint) Shutdown() { - if atomic.SwapUint32(&ep.shutdown, 1) != 0 { + if ep.shutdown.Swap(1) != 0 { // ep.Shutdown() has previously been called. return } @@ -153,7 +152,7 @@ func (ep *Endpoint) Shutdown() { // isShutdownLocally returns true if ep.Shutdown() has been called. func (ep *Endpoint) isShutdownLocally() bool { - return atomic.LoadUint32(&ep.shutdown) != 0 + return ep.shutdown.Load() != 0 } // ShutdownError is returned by most Endpoint methods after Endpoint.Shutdown() @@ -204,7 +203,7 @@ func (ep *Endpoint) RecvFirst() (uint32, error) { return 0, err } raceBecomeActive() - recvDataLen := atomic.LoadUint32(ep.dataLen()) + recvDataLen := ep.dataLen().Load() if recvDataLen > ep.dataCap { return 0, fmt.Errorf("received packet with invalid datagram length %d (maximum %d)", recvDataLen, ep.dataCap) } @@ -248,13 +247,13 @@ func (ep *Endpoint) sendRecv(dataLen uint32, mayRetainP bool) (uint32, error) { // synchronize with the receiver. We will not read from ep.dataLen() until // after ep.ctrlRoundTrip(), so if the peer is mutating it concurrently then // they can only shoot themselves in the foot. - *ep.dataLen() = dataLen + ep.dataLen().RacyStore(dataLen) raceBecomeInactive() if err := ep.ctrlRoundTrip(mayRetainP); err != nil { return 0, err } raceBecomeActive() - recvDataLen := atomic.LoadUint32(ep.dataLen()) + recvDataLen := ep.dataLen().Load() if recvDataLen > ep.dataCap { return 0, fmt.Errorf("received packet with invalid datagram length %d (maximum %d)", recvDataLen, ep.dataCap) } @@ -274,7 +273,7 @@ func (ep *Endpoint) SendLast(dataLen uint32) error { if dataLen > ep.dataCap { panic(fmt.Sprintf("attempting to send packet with datagram length %d (maximum %d)", dataLen, ep.dataCap)) } - *ep.dataLen() = dataLen + ep.dataLen().RacyStore(dataLen) raceBecomeInactive() if err := ep.ctrlWakeLast(); err != nil { return err diff --git a/pkg/flipcall/flipcall_unsafe.go b/pkg/flipcall/flipcall_unsafe.go index 613ed8943..547fda618 100644 --- a/pkg/flipcall/flipcall_unsafe.go +++ b/pkg/flipcall/flipcall_unsafe.go @@ -18,6 +18,7 @@ import ( "reflect" "unsafe" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/sync" ) @@ -40,12 +41,12 @@ const ( PacketHeaderBytes = 16 ) -func (ep *Endpoint) connState() *uint32 { - return (*uint32)(unsafe.Pointer(ep.packet)) +func (ep *Endpoint) connState() *atomicbitops.Uint32 { + return (*atomicbitops.Uint32)(unsafe.Pointer(ep.packet)) } -func (ep *Endpoint) dataLen() *uint32 { - return (*uint32)(unsafe.Pointer(ep.packet + 4)) +func (ep *Endpoint) dataLen() *atomicbitops.Uint32 { + return (*atomicbitops.Uint32)(unsafe.Pointer(ep.packet + 4)) } // Data returns the datagram part of ep's packet window as a byte slice. diff --git a/pkg/flipcall/futex_linux.go b/pkg/flipcall/futex_linux.go index 4bb85939b..97129c365 100644 --- a/pkg/flipcall/futex_linux.go +++ b/pkg/flipcall/futex_linux.go @@ -20,17 +20,16 @@ package flipcall import ( "fmt" "runtime" - "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" ) func (ep *Endpoint) futexSetPeerActive() error { - if atomic.CompareAndSwapUint32(ep.connState(), ep.activeState, ep.inactiveState) { + if ep.connState().CompareAndSwap(ep.activeState, ep.inactiveState) { return nil } - switch cs := atomic.LoadUint32(ep.connState()); cs { + switch cs := ep.connState().Load(); cs { case csShutdown: return ShutdownError{} default: @@ -47,7 +46,7 @@ func (ep *Endpoint) futexWakePeer() error { func (ep *Endpoint) futexWaitUntilActive() error { for { - switch cs := atomic.LoadUint32(ep.connState()); cs { + switch cs := ep.connState().Load(); cs { case ep.activeState: return nil case ep.inactiveState: diff --git a/pkg/lisafs/node.go b/pkg/lisafs/node.go index 053237be2..0bf5c8ebd 100644 --- a/pkg/lisafs/node.go +++ b/pkg/lisafs/node.go @@ -15,8 +15,7 @@ package lisafs import ( - "sync/atomic" - + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/fspath" "gvisor.dev/gvisor/pkg/sync" @@ -69,7 +68,7 @@ type Node struct { // anymore. This node may have been replaced with something hazardous. // deleted is protected by opMu. deleted must only be accessed/mutated using // atomics; see markDeletedRecursive for more details. - deleted uint32 + deleted atomicbitops.Uint32 // name is the name of the file represented by this Node in parent. If this // FD represents the root directory, then name is an empty string. name is @@ -185,7 +184,7 @@ func (n *Node) FilePath() string { } func (n *Node) isDeleted() bool { - return atomic.LoadUint32(&n.deleted) != 0 + return n.deleted.Load() != 0 } func (n *Node) removeFD(fd *ControlFD) { @@ -279,7 +278,7 @@ func (n *Node) forEachChild(fn func(*Node)) { // Precondition: opMu must be locked for writing on the root node being marked // as deleted. func (n *Node) markDeletedRecursive() { - atomic.StoreUint32(&n.deleted, 1) + n.deleted.Store(1) // No need to hold opMu for children as it introduces lock ordering issues // because forEachChild locks childrenMu. Locking opMu after childrenMu diff --git a/pkg/log/log.go b/pkg/log/log.go index 073cf6238..478782a9f 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -29,6 +29,8 @@ // This is because the log.Debugf(...) statement alone will generate a // significant amount of garbage and churn in many cases, even if no log // message is ultimately emitted. +// +// +checkalignedignore package log import ( @@ -92,7 +94,8 @@ type Writer struct { // errors counts failures to write log messages so it can be reported // when writer start to work again. Needs to be accessed using atomics // to make race detector happy because it's read outside the mutex. - errors int32 + // +checklocks + atomicErrors int32 } // Write writes out the given bytes, handling non-blocking sockets. @@ -112,7 +115,7 @@ func (l *Writer) Write(data []byte) (int, error) { // Some other error? if err != nil { l.mu.Lock() - atomic.AddInt32(&l.errors, 1) + atomic.AddInt32(&l.atomicErrors, 1) l.mu.Unlock() return n, err } @@ -124,15 +127,15 @@ func (l *Writer) Write(data []byte) (int, error) { } // Dirty read in case there were errors (rare). - if atomic.LoadInt32(&l.errors) > 0 { + if atomic.LoadInt32(&l.atomicErrors) > 0 { l.mu.Lock() defer l.mu.Unlock() // Recheck condition under lock. - if e := atomic.LoadInt32(&l.errors); e > 0 { + if e := atomic.LoadInt32(&l.atomicErrors); e > 0 { msg := fmt.Sprintf("\n*** Dropped %d log messages ***\n", e) if _, err := l.Next.Write([]byte(msg)); err == nil { - atomic.StoreInt32(&l.errors, 0) + atomic.StoreInt32(&l.atomicErrors, 0) } } } diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index fed893934..236b230ac 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -18,9 +18,9 @@ import ( "errors" "fmt" "io" - "sync/atomic" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/log" ) @@ -64,12 +64,12 @@ type clientFile struct { fid FID // closed indicates whether this file has been closed. - closed uint32 + closed atomicbitops.Uint32 } // Walk implements File.Walk. func (c *clientFile) Walk(names []string) ([]QID, File, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, nil, unix.EBADF } @@ -90,7 +90,7 @@ func (c *clientFile) Walk(names []string) ([]QID, File, error) { // WalkGetAttr implements File.WalkGetAttr. func (c *clientFile) WalkGetAttr(components []string) ([]QID, File, AttrMask, Attr, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, nil, AttrMask{}, Attr{}, unix.EBADF } @@ -123,7 +123,7 @@ func (c *clientFile) WalkGetAttr(components []string) ([]QID, File, AttrMask, At } func (c *clientFile) MultiGetAttr(names []string) ([]FullStat, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, unix.EBADF } @@ -183,7 +183,7 @@ func (c *clientFile) MultiGetAttr(names []string) ([]FullStat, error) { // StatFS implements File.StatFS. func (c *clientFile) StatFS() (FSStat, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return FSStat{}, unix.EBADF } @@ -197,7 +197,7 @@ func (c *clientFile) StatFS() (FSStat, error) { // FSync implements File.FSync. func (c *clientFile) FSync() error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -206,7 +206,7 @@ func (c *clientFile) FSync() error { // GetAttr implements File.GetAttr. func (c *clientFile) GetAttr(req AttrMask) (QID, AttrMask, Attr, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return QID{}, AttrMask{}, Attr{}, unix.EBADF } @@ -220,7 +220,7 @@ func (c *clientFile) GetAttr(req AttrMask) (QID, AttrMask, Attr, error) { // SetAttr implements File.SetAttr. func (c *clientFile) SetAttr(valid SetAttrMask, attr SetAttr) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -229,7 +229,7 @@ func (c *clientFile) SetAttr(valid SetAttrMask, attr SetAttr) error { // GetXattr implements File.GetXattr. func (c *clientFile) GetXattr(name string, size uint64) (string, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return "", unix.EBADF } if !versionSupportsGetSetXattr(c.client.version) { @@ -246,7 +246,7 @@ func (c *clientFile) GetXattr(name string, size uint64) (string, error) { // SetXattr implements File.SetXattr. func (c *clientFile) SetXattr(name, value string, flags uint32) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } if !versionSupportsGetSetXattr(c.client.version) { @@ -258,7 +258,7 @@ func (c *clientFile) SetXattr(name, value string, flags uint32) error { // ListXattr implements File.ListXattr. func (c *clientFile) ListXattr(size uint64) (map[string]struct{}, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, unix.EBADF } if !versionSupportsListRemoveXattr(c.client.version) { @@ -279,7 +279,7 @@ func (c *clientFile) ListXattr(size uint64) (map[string]struct{}, error) { // RemoveXattr implements File.RemoveXattr. func (c *clientFile) RemoveXattr(name string) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } if !versionSupportsListRemoveXattr(c.client.version) { @@ -291,7 +291,7 @@ func (c *clientFile) RemoveXattr(name string) error { // Allocate implements File.Allocate. func (c *clientFile) Allocate(mode AllocateMode, offset, length uint64) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } if !versionSupportsTallocate(c.client.version) { @@ -307,7 +307,7 @@ func (c *clientFile) Allocate(mode AllocateMode, offset, length uint64) error { // considered deprecated. func (c *clientFile) Remove() error { // Avoid double close. - if !atomic.CompareAndSwapUint32(&c.closed, 0, 1) { + if !c.closed.CompareAndSwap(0, 1) { return unix.EBADF } @@ -328,7 +328,7 @@ func (c *clientFile) Remove() error { // Close implements File.Close. func (c *clientFile) Close() error { // Avoid double close. - if !atomic.CompareAndSwapUint32(&c.closed, 0, 1) { + if !c.closed.CompareAndSwap(0, 1) { return unix.EBADF } @@ -361,7 +361,7 @@ func (c *clientFile) SetAttrClose(valid SetAttrMask, attr SetAttr) error { } // Avoid double close. - if !atomic.CompareAndSwapUint32(&c.closed, 0, 1) { + if !c.closed.CompareAndSwap(0, 1) { return unix.EBADF } @@ -380,7 +380,7 @@ func (c *clientFile) SetAttrClose(valid SetAttrMask, attr SetAttr) error { // Open implements File.Open. func (c *clientFile) Open(flags OpenFlags) (*fd.FD, QID, uint32, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, QID{}, 0, unix.EBADF } @@ -393,7 +393,7 @@ func (c *clientFile) Open(flags OpenFlags) (*fd.FD, QID, uint32, error) { } func (c *clientFile) Bind(sockType uint32, sockName string, uid UID, gid GID) (File, QID, AttrMask, Attr, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, QID{}, AttrMask{}, Attr{}, unix.EBADF } @@ -425,7 +425,7 @@ func (c *clientFile) Bind(sockType uint32, sockName string, uid UID, gid GID) (F // Connect implements File.Connect. func (c *clientFile) Connect(socketType SocketType) (*fd.FD, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, unix.EBADF } @@ -494,7 +494,7 @@ func (c *clientFile) ReadAt(p []byte, offset uint64) (int, error) { } func (c *clientFile) readAt(p []byte, offset uint64) (int, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return 0, unix.EBADF } @@ -525,7 +525,7 @@ func (c *clientFile) WriteAt(p []byte, offset uint64) (int, error) { } func (c *clientFile) writeAt(p []byte, offset uint64) (int, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return 0, unix.EBADF } @@ -590,7 +590,7 @@ func (r *ReadWriterFile) WriteAt(p []byte, offset int64) (int, error) { // Rename implements File.Rename. func (c *clientFile) Rename(dir File, name string) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -604,7 +604,7 @@ func (c *clientFile) Rename(dir File, name string) error { // Create implements File.Create. func (c *clientFile) Create(name string, openFlags OpenFlags, permissions FileMode, uid UID, gid GID) (*fd.FD, File, QID, uint32, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, nil, QID{}, 0, unix.EBADF } @@ -635,7 +635,7 @@ func (c *clientFile) Create(name string, openFlags OpenFlags, permissions FileMo // Mkdir implements File.Mkdir. func (c *clientFile) Mkdir(name string, permissions FileMode, uid UID, gid GID) (QID, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return QID{}, unix.EBADF } @@ -665,7 +665,7 @@ func (c *clientFile) Mkdir(name string, permissions FileMode, uid UID, gid GID) // Symlink implements File.Symlink. func (c *clientFile) Symlink(oldname string, newname string, uid UID, gid GID) (QID, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return QID{}, unix.EBADF } @@ -695,7 +695,7 @@ func (c *clientFile) Symlink(oldname string, newname string, uid UID, gid GID) ( // Link implements File.Link. func (c *clientFile) Link(target File, newname string) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -709,7 +709,7 @@ func (c *clientFile) Link(target File, newname string) error { // Mknod implements File.Mknod. func (c *clientFile) Mknod(name string, mode FileMode, major uint32, minor uint32, uid UID, gid GID) (QID, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return QID{}, unix.EBADF } @@ -741,7 +741,7 @@ func (c *clientFile) Mknod(name string, mode FileMode, major uint32, minor uint3 // RenameAt implements File.RenameAt. func (c *clientFile) RenameAt(oldname string, newdir File, newname string) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -755,7 +755,7 @@ func (c *clientFile) RenameAt(oldname string, newdir File, newname string) error // UnlinkAt implements File.UnlinkAt. func (c *clientFile) UnlinkAt(name string, flags uint32) error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } @@ -764,7 +764,7 @@ func (c *clientFile) UnlinkAt(name string, flags uint32) error { // Readdir implements File.Readdir. func (c *clientFile) Readdir(offset uint64, count uint32) ([]Dirent, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return nil, unix.EBADF } @@ -778,7 +778,7 @@ func (c *clientFile) Readdir(offset uint64, count uint32) ([]Dirent, error) { // Readlink implements File.Readlink. func (c *clientFile) Readlink() (string, error) { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return "", unix.EBADF } @@ -792,7 +792,7 @@ func (c *clientFile) Readlink() (string, error) { // Flush implements File.Flush. func (c *clientFile) Flush() error { - if atomic.LoadUint32(&c.closed) != 0 { + if c.closed.Load() != 0 { return unix.EBADF } diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 6e966b6d2..a4f1c6f74 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -21,7 +21,6 @@ import ( "os" "path" "strings" - "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -127,7 +126,7 @@ func (t *Tversion) handle(cs *connState) message { if t.MSize > maximumLength { return newErr(unix.EINVAL) } - atomic.StoreUint32(&cs.messageSize, t.MSize) + cs.messageSize.Store(t.MSize) requested, ok := parseVersion(t.Version) if !ok { return newErr(unix.EINVAL) @@ -139,7 +138,7 @@ func (t *Tversion) handle(cs *connState) message { } // From Tversion(9P): "The server may respond with the client’s version // string, or a version string identifying an earlier defined protocol version". - atomic.StoreUint32(&cs.version, requested) + cs.version.Store(requested) return &Rversion{ MSize: t.MSize, Version: t.Version, @@ -1583,7 +1582,7 @@ func (t *Tmultigetattr) handle(cs *connState) message { } parentNode.opMu.RLock() - if atomic.LoadUint32(&parentNode.deleted) != 0 { + if parentNode.deleted.Load() != 0 { parentNode.opMu.RUnlock() break } diff --git a/pkg/p9/path_tree.go b/pkg/p9/path_tree.go index 9b779418c..e514440c5 100644 --- a/pkg/p9/path_tree.go +++ b/pkg/p9/path_tree.go @@ -17,6 +17,7 @@ package p9 import ( "fmt" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/sync" ) @@ -40,7 +41,7 @@ type pathNode struct { // already been unlinked. deleted is protected by opMu. However, it may be // changed without opMu if this node is deleted as part of an entire subtree // on unlink. So deleted must only be accessed/mutated using atomics. - deleted uint32 + deleted atomicbitops.Uint32 // childMu protects the fields below. childMu sync.RWMutex diff --git a/pkg/p9/server.go b/pkg/p9/server.go index 841a7f5d5..4d3463d74 100644 --- a/pkg/p9/server.go +++ b/pkg/p9/server.go @@ -17,7 +17,6 @@ package p9 import ( "io" "runtime/debug" - "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -84,11 +83,11 @@ type connState struct { // messageSize is the maximum message size. The server does not // do automatic splitting of messages. - messageSize uint32 + messageSize atomicbitops.Uint32 // version is the agreed upon version X of 9P2000.L.Google.X. // version 0 implies 9P2000.L. - version uint32 + version atomicbitops.Uint32 // reqGate counts requests that are still being handled. reqGate sync.Gate @@ -99,9 +98,8 @@ type connState struct { recvMu sync.Mutex // recvIdle is the number of goroutines in handleRequests() attempting to - // lock recvMu so that they can receive from conn. recvIdle is accessed - // using atomic memory operations. - recvIdle int32 + // lock recvMu so that they can receive from conn. + recvIdle atomicbitops.Int32 // If recvShutdown is true, at least one goroutine has observed a // connection error while receiving from conn, and all goroutines in @@ -225,7 +223,7 @@ func (f *fidRef) TryIncRef() bool { // Precondition: this must be called via safelyRead, safelyWrite or // safelyGlobal. func (f *fidRef) isDeleted() bool { - return atomic.LoadUint32(&f.pathNode.deleted) != 0 + return f.pathNode.deleted.Load() != 0 } // isRoot indicates whether this is a root fid. @@ -245,7 +243,7 @@ func (f *fidRef) maybeParent() *fidRef { // // Precondition: this must be called via safelyWrite or safelyGlobal. func notifyDelete(pn *pathNode) { - atomic.StoreUint32(&pn.deleted, 1) + pn.deleted.Store(1) // Call on all subtrees. pn.forEachChildNode(func(pn *pathNode) { @@ -537,9 +535,9 @@ func (cs *connState) handle(m message) (r message) { // continue handling requests and false if it should terminate. func (cs *connState) handleRequest() bool { // Obtain the right to receive a message from cs.conn. - atomic.AddInt32(&cs.recvIdle, 1) + cs.recvIdle.Add(1) cs.recvMu.Lock() - atomic.AddInt32(&cs.recvIdle, -1) + cs.recvIdle.Add(-1) if cs.recvShutdown { // Another goroutine already detected a connection problem; exit @@ -548,7 +546,7 @@ func (cs *connState) handleRequest() bool { return false } - messageSize := atomic.LoadUint32(&cs.messageSize) + messageSize := cs.messageSize.Load() if messageSize == 0 { // Default or not yet negotiated. messageSize = maximumLength @@ -565,7 +563,7 @@ func (cs *connState) handleRequest() bool { } // Ensure that another goroutine is available to receive from cs.conn. - if atomic.LoadInt32(&cs.recvIdle) == 0 { + if cs.recvIdle.Load() == 0 { go cs.handleRequests() // S/R-SAFE: Irrelevant. } cs.recvMu.Unlock() diff --git a/pkg/refs/refcounter.go b/pkg/refs/refcounter.go index d9cb87511..2bdf6a10c 100644 --- a/pkg/refs/refcounter.go +++ b/pkg/refs/refcounter.go @@ -289,16 +289,16 @@ func (l LeakMode) String() string { // Values must be one of the LeakMode values. // // leakMode must be accessed atomically. -var leakMode uint32 +var leakMode atomicbitops.Uint32 // SetLeakMode configures the reference leak checker. func SetLeakMode(mode LeakMode) { - atomic.StoreUint32(&leakMode, uint32(mode)) + leakMode.Store(uint32(mode)) } // GetLeakMode returns the current leak mode. func GetLeakMode() LeakMode { - return LeakMode(atomic.LoadUint32(&leakMode)) + return LeakMode(leakMode.Load()) } const maxStackFrames = 40 @@ -375,7 +375,7 @@ func FormatStack(pcs []uintptr) string { func (r *AtomicRefCount) finalize() { var note string - switch LeakMode(atomic.LoadUint32(&leakMode)) { + switch LeakMode(leakMode.Load()) { case NoLeakChecking: return case UninitializedLeakChecking: @@ -405,7 +405,7 @@ func (r *AtomicRefCount) EnableLeakCheck(name string) { if name == "" { panic("invalid name") } - switch LeakMode(atomic.LoadUint32(&leakMode)) { + switch LeakMode(leakMode.Load()) { case NoLeakChecking: return case LeaksLogTraces: @@ -533,7 +533,7 @@ func (r *AtomicRefCount) DecRef(ctx context.Context) { // finalizer will run before exiting, but this at least ensures that they will // be discovered/enqueued by GC. func OnExit() { - if LeakMode(atomic.LoadUint32(&leakMode)) != NoLeakChecking { + if LeakMode(leakMode.Load()) != NoLeakChecking { runtime.GC() } } diff --git a/pkg/sentry/contexttest/contexttest.go b/pkg/sentry/contexttest/contexttest.go index 5debc589f..eadd23e98 100644 --- a/pkg/sentry/contexttest/contexttest.go +++ b/pkg/sentry/contexttest/contexttest.go @@ -17,7 +17,6 @@ package contexttest import ( "os" - "sync/atomic" "testing" "time" @@ -88,8 +87,8 @@ func (*globalUniqueIDProvider) UniqueID() uint64 { } // lastInotifyCookie is a monotonically increasing counter for generating unique -// inotify cookies. Must be accessed using atomic ops. -var lastInotifyCookie uint32 +// inotify cookies. +var lastInotifyCookie atomicbitops.Uint32 // hostClock implements ktime.Clock. type hostClock struct { @@ -126,7 +125,7 @@ func (t *TestContext) Value(key interface{}) interface{} { case uniqueid.CtxGlobalUniqueIDProvider: return &globalUniqueIDProvider{} case uniqueid.CtxInotifyCookie: - return atomic.AddUint32(&lastInotifyCookie, 1) + return lastInotifyCookie.Add(1) case ktime.CtxRealtimeClock: return &hostClock{} default: diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go index d300a32e0..f94158347 100644 --- a/pkg/sentry/fs/dirent.go +++ b/pkg/sentry/fs/dirent.go @@ -17,10 +17,10 @@ package fs import ( "fmt" "path" - "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/refs" @@ -118,7 +118,7 @@ type Dirent struct { parent *Dirent // deleted may be set atomically when removed. - deleted int32 + deleted atomicbitops.Int32 // mounted is true if Dirent is a mount point, similar to include/linux/dcache.h:DCACHE_MOUNTED. mounted bool @@ -373,7 +373,7 @@ func (d *Dirent) fullName(root *Dirent) (string, bool) { d.parent.mu.Unlock() parentName, reachable := d.parent.fullName(root) s := path.Join(parentName, name) - if atomic.LoadInt32(&d.deleted) != 0 { + if d.deleted.Load() != 0 { return s + " (deleted)", reachable } return s, reachable @@ -961,7 +961,7 @@ func (d *Dirent) isMountPointLocked() bool { // Precondition: must be called with mm.withMountLocked held on `d`. func (d *Dirent) mount(ctx context.Context, inode *Inode) (newChild *Dirent, err error) { // Did we race with deletion? - if atomic.LoadInt32(&d.deleted) != 0 { + if d.deleted.Load() != 0 { return nil, linuxerr.ENOENT } @@ -996,7 +996,7 @@ func (d *Dirent) mount(ctx context.Context, inode *Inode) (newChild *Dirent, err // Precondition: must be called with mm.withMountLocked held on `d`. func (d *Dirent) unmount(ctx context.Context, replacement *Dirent) error { // Did we race with deletion? - if atomic.LoadInt32(&d.deleted) != 0 { + if d.deleted.Load() != 0 { return linuxerr.ENOENT } @@ -1058,7 +1058,7 @@ func (d *Dirent) Remove(ctx context.Context, root *Dirent, name string, dirPath child.Inode.Watches.Notify("", linux.IN_ATTRIB, 0) // Mark name as deleted and remove from children. - atomic.StoreInt32(&child.deleted, 1) + child.deleted.Store(1) if w, ok := d.children[name]; ok { delete(d.children, name) w.Drop(ctx) @@ -1124,7 +1124,7 @@ func (d *Dirent) RemoveDirectory(ctx context.Context, root *Dirent, name string) } // Mark name as deleted and remove from children. - atomic.StoreInt32(&child.deleted, 1) + child.deleted.Store(1) if w, ok := d.children[name]; ok { delete(d.children, name) w.Drop(ctx) diff --git a/pkg/sentry/fs/dirent_state.go b/pkg/sentry/fs/dirent_state.go index 67a35f0b2..7aaf8918f 100644 --- a/pkg/sentry/fs/dirent_state.go +++ b/pkg/sentry/fs/dirent_state.go @@ -16,7 +16,6 @@ package fs import ( "fmt" - "sync/atomic" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/refs" @@ -37,7 +36,7 @@ func (d *Dirent) beforeSave() { // perfectly OK to save---we are simply disallowing it here to prevent // generating non-restorable state dumps. As the program continues its // execution, it may become allowed to save again. - if !d.Inode.IsVirtual() && atomic.LoadInt32(&d.deleted) != 0 { + if !d.Inode.IsVirtual() && d.deleted.Load() != 0 { n, _ := d.FullName(nil /* root */) panic(ErrSaveRejection{fmt.Errorf("deleted file %q still has open fds", n)}) } diff --git a/pkg/sentry/fs/inotify.go b/pkg/sentry/fs/inotify.go index a164afe2a..1b8a9a5fe 100644 --- a/pkg/sentry/fs/inotify.go +++ b/pkg/sentry/fs/inotify.go @@ -16,9 +16,9 @@ package fs import ( "io" - "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/hostarch" @@ -258,7 +258,7 @@ func (i *Inotify) newWatchLocked(target *Dirent, mask uint32) *Watch { watch := &Watch{ owner: i, wd: wd, - mask: mask, + mask: atomicbitops.FromUint32(mask), target: target.Inode, pins: make(map[*Dirent]bool), } @@ -307,9 +307,9 @@ func (i *Inotify) AddWatch(target *Dirent, mask uint32) int32 { if mergeMask := mask&linux.IN_MASK_ADD != 0; mergeMask { // "Add (OR) events to watch mask for this pathname if it already // exists (instead of replacing mask)." -- inotify(7) - newmask |= atomic.LoadUint32(&existing.mask) + newmask |= existing.mask.Load() } - atomic.StoreUint32(&existing.mask, newmask) + existing.mask.Store(newmask) return existing.wd } diff --git a/pkg/sentry/fs/inotify_watch.go b/pkg/sentry/fs/inotify_watch.go index 605423d22..fce1f791d 100644 --- a/pkg/sentry/fs/inotify_watch.go +++ b/pkg/sentry/fs/inotify_watch.go @@ -15,9 +15,8 @@ package fs import ( - "sync/atomic" - "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/sync" ) @@ -51,7 +50,7 @@ type Watch struct { // Events being monitored via this watch. Must be accessed atomically, // writes are protected by mu. - mask uint32 + mask atomicbitops.Uint32 // pins is the set of dirents this watch is currently pinning in memory by // holding a reference to them. See Pin()/Unpin(). @@ -67,7 +66,7 @@ func (w *Watch) ID() uint64 { // should continue to be be notified of events after the target has been // unlinked. func (w *Watch) NotifyParentAfterUnlink() bool { - return atomic.LoadUint32(&w.mask)&linux.IN_EXCL_UNLINK == 0 + return w.mask.Load()&linux.IN_EXCL_UNLINK == 0 } // isRenameEvent returns true if eventMask describes a rename event. @@ -77,7 +76,7 @@ func isRenameEvent(eventMask uint32) bool { // Notify queues a new event on this watch. func (w *Watch) Notify(name string, events uint32, cookie uint32) { - mask := atomic.LoadUint32(&w.mask) + mask := w.mask.Load() if mask&events == 0 { // We weren't watching for this event. return diff --git a/pkg/sentry/mm/address_space.go b/pkg/sentry/mm/address_space.go index 534e0e957..168b267b9 100644 --- a/pkg/sentry/mm/address_space.go +++ b/pkg/sentry/mm/address_space.go @@ -16,7 +16,6 @@ package mm import ( "fmt" - "sync/atomic" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/hostarch" @@ -27,7 +26,7 @@ import ( // // Preconditions: The caller must have called mm.Activate(). func (mm *MemoryManager) AddressSpace() platform.AddressSpace { - if atomic.LoadInt32(&mm.active) == 0 { + if mm.active.Load() == 0 { panic("trying to use inactive address space?") } return mm.as @@ -43,12 +42,12 @@ func (mm *MemoryManager) Activate(ctx context.Context) error { // Fast path: the MemoryManager already has an active // platform.AddressSpace, and we just need to indicate that we need it too. for { - active := atomic.LoadInt32(&mm.active) + active := mm.active.Load() if active == 0 { // Fall back to the slow path. break } - if atomic.CompareAndSwapInt32(&mm.active, active, active+1) { + if mm.active.CompareAndSwap(active, active+1) { return nil } } @@ -61,10 +60,10 @@ func (mm *MemoryManager) Activate(ctx context.Context) error { // method is commonly in the hot-path. // Check if we raced with another goroutine performing activation. - if atomic.LoadInt32(&mm.active) > 0 { + if mm.active.Load() > 0 { // This can't race; Deactivate can't decrease mm.active from 1 to 0 // without holding activeMu. - atomic.AddInt32(&mm.active, 1) + mm.active.Add(1) mm.activeMu.Unlock() return nil } @@ -72,7 +71,7 @@ func (mm *MemoryManager) Activate(ctx context.Context) error { // Do we have a context? If so, then we never unmapped it. This can // only be the case if !mm.p.CooperativelySchedulesAddressSpace(). if mm.as != nil { - atomic.StoreInt32(&mm.active, 1) + mm.active.Store(1) mm.activeMu.Unlock() return nil } @@ -118,7 +117,7 @@ func (mm *MemoryManager) Activate(ctx context.Context) error { // Now that m.as has been assigned, we can set m.active to a non-zero value // to enable the fast path. - atomic.StoreInt32(&mm.active, 1) + mm.active.Store(1) mm.activeMu.Unlock() return nil @@ -130,12 +129,12 @@ func (mm *MemoryManager) Deactivate() { // Fast path: this is not the last goroutine to deactivate the // MemoryManager. for { - active := atomic.LoadInt32(&mm.active) + active := mm.active.Load() if active == 1 { // Fall back to the slow path. break } - if atomic.CompareAndSwapInt32(&mm.active, active, active-1) { + if mm.active.CompareAndSwap(active, active-1) { return } } @@ -144,7 +143,7 @@ func (mm *MemoryManager) Deactivate() { // Same as Activate. // Still active? - if atomic.AddInt32(&mm.active, -1) > 0 { + if mm.active.Add(-1) > 0 { mm.activeMu.Unlock() return } diff --git a/pkg/sentry/mm/lifecycle.go b/pkg/sentry/mm/lifecycle.go index 2e59e415d..32bad1172 100644 --- a/pkg/sentry/mm/lifecycle.go +++ b/pkg/sentry/mm/lifecycle.go @@ -16,8 +16,8 @@ package mm import ( "fmt" - "sync/atomic" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -34,7 +34,7 @@ func NewMemoryManager(p platform.Platform, mfp pgalloc.MemoryFileProvider, sleep mfp: mfp, haveASIO: p.SupportsAddressSpaceIO(), privateRefs: &privateRefs{}, - users: 1, + users: atomicbitops.FromInt32(1), auxv: arch.Auxv{}, dumpability: UserDumpable, aioManager: aioManager{contexts: make(map[uint64]*AIOContext)}, @@ -78,7 +78,7 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { haveASIO: mm.haveASIO, layout: mm.layout, privateRefs: mm.privateRefs, - users: 1, + users: atomicbitops.FromInt32(1), brk: mm.brk, usageAS: mm.usageAS, dataAS: mm.dataAS, @@ -243,11 +243,11 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { // already 0, IncUsers does nothing and returns false. func (mm *MemoryManager) IncUsers() bool { for { - users := atomic.LoadInt32(&mm.users) + users := mm.users.Load() if users == 0 { return false } - if atomic.CompareAndSwapInt32(&mm.users, users, users+1) { + if mm.users.CompareAndSwap(users, users+1) { return true } } @@ -256,7 +256,7 @@ func (mm *MemoryManager) IncUsers() bool { // DecUsers decrements mm's user count. If the user count reaches 0, all // mappings in mm are unmapped. func (mm *MemoryManager) DecUsers(ctx context.Context) { - if users := atomic.AddInt32(&mm.users, -1); users > 0 { + if users := mm.users.Add(-1); users > 0 { return } else if users < 0 { panic(fmt.Sprintf("Invalid MemoryManager.users: %d", users)) @@ -274,7 +274,7 @@ func (mm *MemoryManager) DecUsers(ctx context.Context) { mm.activeMu.Lock() // Sanity check. - if atomic.LoadInt32(&mm.active) != 0 { + if mm.active.Load() != 0 { panic("active address space lost?") } // Make sure the AddressSpace is returned. diff --git a/pkg/sentry/mm/mm.go b/pkg/sentry/mm/mm.go index 095f8efd2..aec322558 100644 --- a/pkg/sentry/mm/mm.go +++ b/pkg/sentry/mm/mm.go @@ -39,6 +39,7 @@ import ( "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/safemem" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -79,9 +80,7 @@ type MemoryManager struct { // users is the number of dependencies on the mappings in the MemoryManager. // When the number of references in users reaches zero, all mappings are // unmapped. - // - // users is accessed using atomic memory operations. - users int32 + users atomicbitops.Int32 // mappingMu is analogous to Linux's struct mm_struct::mmap_sem. mappingMu sync.RWMutex `state:"nosave"` @@ -169,7 +168,7 @@ type MemoryManager struct { // activeMu. (This is because such transitions may need to be atomic with // changes to as.) as platform.AddressSpace `state:"nosave"` - active int32 `state:"zerovalue"` + active atomicbitops.Int32 `state:"zerovalue"` // unmapAllOnActivate indicates that the next Activate call should activate // an empty AddressSpace. @@ -243,15 +242,11 @@ type MemoryManager struct { // previously been called. Since, as of this writing, // MEMBARRIER_CMD_PRIVATE_EXPEDITED is implemented as a global memory // barrier, membarrierPrivateEnabled has no other effect. - // - // membarrierPrivateEnabled is accessed using atomic memory operations. - membarrierPrivateEnabled uint32 + membarrierPrivateEnabled atomicbitops.Uint32 // membarrierRSeqEnabled is non-zero if EnableMembarrierRSeq has previously // been called. - // - // membarrierRSeqEnabled is accessed using atomic memory operations. - membarrierRSeqEnabled uint32 + membarrierRSeqEnabled atomicbitops.Uint32 } // vma represents a virtual memory area. diff --git a/pkg/sentry/mm/syscalls.go b/pkg/sentry/mm/syscalls.go index e4c889081..911a58f47 100644 --- a/pkg/sentry/mm/syscalls.go +++ b/pkg/sentry/mm/syscalls.go @@ -17,7 +17,6 @@ package mm import ( "fmt" mrand "math/rand" - "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/context" @@ -1301,23 +1300,23 @@ func (mm *MemoryManager) VirtualDataSize() uint64 { // EnableMembarrierPrivate causes future calls to IsMembarrierPrivateEnabled to // return true. func (mm *MemoryManager) EnableMembarrierPrivate() { - atomic.StoreUint32(&mm.membarrierPrivateEnabled, 1) + mm.membarrierPrivateEnabled.Store(1) } // IsMembarrierPrivateEnabled returns true if mm.EnableMembarrierPrivate() has // previously been called. func (mm *MemoryManager) IsMembarrierPrivateEnabled() bool { - return atomic.LoadUint32(&mm.membarrierPrivateEnabled) != 0 + return mm.membarrierPrivateEnabled.Load() != 0 } // EnableMembarrierRSeq causes future calls to IsMembarrierRSeqEnabled to // return true. func (mm *MemoryManager) EnableMembarrierRSeq() { - atomic.StoreUint32(&mm.membarrierRSeqEnabled, 1) + mm.membarrierRSeqEnabled.Store(1) } // IsMembarrierRSeqEnabled returns true if mm.EnableMembarrierRSeq() has // previously been called. func (mm *MemoryManager) IsMembarrierRSeqEnabled() bool { - return atomic.LoadUint32(&mm.membarrierRSeqEnabled) != 0 + return mm.membarrierRSeqEnabled.Load() != 0 } diff --git a/pkg/sentry/pgalloc/BUILD b/pkg/sentry/pgalloc/BUILD index 496a9fd97..faca66d6e 100644 --- a/pkg/sentry/pgalloc/BUILD +++ b/pkg/sentry/pgalloc/BUILD @@ -84,6 +84,7 @@ go_library( visibility = ["//pkg/sentry:internal"], deps = [ "//pkg/abi/linux", + "//pkg/atomicbitops", "//pkg/context", "//pkg/errors/linuxerr", "//pkg/hostarch", diff --git a/pkg/sentry/pgalloc/save_restore.go b/pkg/sentry/pgalloc/save_restore.go index 345cdde55..b289e2617 100644 --- a/pkg/sentry/pgalloc/save_restore.go +++ b/pkg/sentry/pgalloc/save_restore.go @@ -20,9 +20,9 @@ import ( "fmt" "io" "runtime" - "sync/atomic" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/usage" @@ -136,11 +136,11 @@ func (f *MemoryFile) LoadFrom(ctx context.Context, r wire.Reader) error { // other since it doesn't do any work between mmaps. The rest of this // function doesn't mutate f.usage, so it's safe to iterate concurrently. mapperDone := make(chan struct{}) - mapperCanceled := int32(0) + mapperCanceled := atomicbitops.FromInt32(0) go func() { // S/R-SAFE: see comment defer func() { close(mapperDone) }() for seg := f.usage.FirstSegment(); seg.Ok(); seg = seg.NextSegment() { - if atomic.LoadInt32(&mapperCanceled) != 0 { + if mapperCanceled.Load() != 0 { return } if seg.Value().knownCommitted { @@ -149,7 +149,7 @@ func (f *MemoryFile) LoadFrom(ctx context.Context, r wire.Reader) error { } }() defer func() { - atomic.StoreInt32(&mapperCanceled, 1) + mapperCanceled.Store(1) <-mapperDone }() diff --git a/pkg/sentry/time/BUILD b/pkg/sentry/time/BUILD index 973577cfc..84cbcf7ce 100644 --- a/pkg/sentry/time/BUILD +++ b/pkg/sentry/time/BUILD @@ -36,6 +36,7 @@ go_library( ], visibility = ["//:sandbox"], deps = [ + "//pkg/atomicbitops", "//pkg/errors/linuxerr", "//pkg/gohacks", "//pkg/log", diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go index cb92b6eee..585ff7a06 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -15,8 +15,7 @@ package vfs import ( - "sync/atomic" - + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/sync" @@ -68,8 +67,7 @@ type Dentry struct { dead bool // mounts is the number of Mounts for which this Dentry is Mount.point. - // mounts is accessed using atomic memory operations. - mounts uint32 + mounts atomicbitops.Uint32 // impl is the DentryImpl associated with this Dentry. impl is immutable. // This should be the last field in Dentry. @@ -166,7 +164,7 @@ func (d *Dentry) IsDead() bool { } func (d *Dentry) isMounted() bool { - return atomic.LoadUint32(&d.mounts) != 0 + return d.mounts.Load() != 0 } // InotifyWithParent notifies all watches on the targets represented by d and diff --git a/pkg/sentry/vfs/file_description.go b/pkg/sentry/vfs/file_description.go index cf8dd0053..0a9970281 100644 --- a/pkg/sentry/vfs/file_description.go +++ b/pkg/sentry/vfs/file_description.go @@ -16,9 +16,9 @@ package vfs import ( "io" - "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/sentry/arch" @@ -51,7 +51,7 @@ type FileDescription struct { // modified by fcntl()" - fcntl(2). statusFlags can be read using atomic // memory operations when it does not need to be synchronized with an // access to asyncHandler. - statusFlags uint32 + statusFlags atomicbitops.Uint32 // asyncHandler handles O_ASYNC signal generation. It is set with the // F_SETOWN or F_SETOWN_EX fcntls. For asyncHandler to be used, O_ASYNC must @@ -84,7 +84,7 @@ type FileDescription struct { // writable is analogous to Linux's FMODE_WRITE. writable bool - usedLockBSD uint32 + usedLockBSD atomicbitops.Uint32 // impl is the FileDescriptionImpl associated with this Filesystem. impl is // immutable. This should be the last field in FileDescription. @@ -142,7 +142,7 @@ func (fd *FileDescription) Init(impl FileDescriptionImpl, flags uint32, mnt *Mou // Remove "file creation flags" to mirror the behavior from file.f_flags in // fs/open.c:do_dentry_open. - fd.statusFlags = flags &^ FileCreationFlags + fd.statusFlags = atomicbitops.FromUint32(flags &^ FileCreationFlags) fd.vd = VirtualDentry{ mount: mnt, dentry: d, @@ -184,7 +184,7 @@ func (fd *FileDescription) DecRef(ctx context.Context) { } // If BSD locks were used, release any lock that it may have acquired. - if atomic.LoadUint32(&fd.usedLockBSD) != 0 { + if fd.usedLockBSD.Load() != 0 { fd.impl.UnlockBSD(context.Background(), fd) } @@ -195,7 +195,7 @@ func (fd *FileDescription) DecRef(ctx context.Context) { } fd.vd.DecRef(ctx) fd.flagsMu.Lock() - if fd.statusFlags&linux.O_ASYNC != 0 && fd.asyncHandler != nil { + if fd.statusFlags.RacyLoad()&linux.O_ASYNC != 0 && fd.asyncHandler != nil { fd.asyncHandler.Unregister(fd) } fd.asyncHandler = nil @@ -228,7 +228,7 @@ func (fd *FileDescription) Options() FileDescriptionOptions { // StatusFlags returns file description status flags, as for fcntl(F_GETFL). func (fd *FileDescription) StatusFlags() uint32 { - return atomic.LoadUint32(&fd.statusFlags) + return fd.statusFlags.Load() } // SetStatusFlags sets file description status flags, as for fcntl(F_SETFL). @@ -279,15 +279,15 @@ func (fd *FileDescription) SetStatusFlags(ctx context.Context, creds *auth.Crede if fd.asyncHandler != nil { // Use fd.statusFlags instead of oldFlags, which may have become outdated, // to avoid double registering/unregistering. - if fd.statusFlags&linux.O_ASYNC == 0 && flags&linux.O_ASYNC != 0 { + if fd.statusFlags.RacyLoad()&linux.O_ASYNC == 0 && flags&linux.O_ASYNC != 0 { if err := fd.asyncHandler.Register(fd); err != nil { return err } - } else if fd.statusFlags&linux.O_ASYNC != 0 && flags&linux.O_ASYNC == 0 { + } else if fd.statusFlags.RacyLoad()&linux.O_ASYNC != 0 && flags&linux.O_ASYNC == 0 { fd.asyncHandler.Unregister(fd) } } - atomic.StoreUint32(&fd.statusFlags, (oldFlags&^settableFlags)|(flags&settableFlags)) + fd.statusFlags.Store((oldFlags &^ settableFlags) | (flags & settableFlags)) fd.flagsMu.Unlock() return nil } @@ -836,7 +836,7 @@ func (fd *FileDescription) SupportsLocks() bool { // LockBSD tries to acquire a BSD-style advisory file lock. func (fd *FileDescription) LockBSD(ctx context.Context, ownerPID int32, lockType lock.LockType, block bool) error { - atomic.StoreUint32(&fd.usedLockBSD, 1) + fd.usedLockBSD.Store(1) return fd.impl.LockBSD(ctx, fd, ownerPID, lockType, block) } @@ -909,7 +909,7 @@ func (fd *FileDescription) SetAsyncHandler(newHandler func() FileAsync) (FileAsy defer fd.flagsMu.Unlock() if fd.asyncHandler == nil { fd.asyncHandler = newHandler() - if fd.statusFlags&linux.O_ASYNC != 0 { + if fd.statusFlags.RacyLoad()&linux.O_ASYNC != 0 { if err := fd.asyncHandler.Register(fd); err != nil { return nil, err } diff --git a/pkg/sentry/vfs/inotify.go b/pkg/sentry/vfs/inotify.go index cd1b4bae6..28b51538d 100644 --- a/pkg/sentry/vfs/inotify.go +++ b/pkg/sentry/vfs/inotify.go @@ -17,9 +17,9 @@ package vfs import ( "bytes" "fmt" - "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/hostarch" @@ -304,7 +304,7 @@ func (i *Inotify) newWatchLocked(d *Dentry, ws *Watches, mask uint32) *Watch { owner: i, wd: i.nextWatchIDLocked(), target: d, - mask: mask, + mask: atomicbitops.FromUint32(mask), } // Hold the watch in this inotify instance as well as the watch set on the @@ -346,9 +346,9 @@ func (i *Inotify) AddWatch(target *Dentry, mask uint32) (int32, error) { if mask&linux.IN_MASK_ADD != 0 { // "Add (OR) events to watch mask for this pathname if it already // exists (instead of replacing mask)." -- inotify(7) - newmask |= atomic.LoadUint32(&existing.mask) + newmask |= existing.mask.Load() } - atomic.StoreUint32(&existing.mask, newmask) + existing.mask.Store(newmask) return existing.wd, nil } @@ -498,7 +498,7 @@ func (w *Watches) cleanupExpiredWatches(ctx context.Context) { var toRemove []*Watch w.mu.RLock() for _, watch := range w.ws { - if atomic.LoadInt32(&watch.expired) == 1 { + if watch.expired.Load() == 1 { toRemove = append(toRemove, watch) } } @@ -563,14 +563,12 @@ type Watch struct { // This field is immutable after creation. target *Dentry - // Events being monitored via this watch. Must be accessed with atomic - // memory operations. - mask uint32 + // Events being monitored via this watch. + mask atomicbitops.Uint32 // expired is set to 1 to indicate that this watch is a one-shot that has - // already sent a notification and therefore can be removed. Must be accessed - // with atomic memory operations. - expired int32 + // already sent a notification and therefore can be removed. + expired atomicbitops.Int32 } // OwnerID returns the id of the inotify instance that owns this watch. @@ -584,20 +582,20 @@ func (w *Watch) OwnerID() uint64 { // For example, if "foo/bar" is opened and then unlinked, operations on the // open fd may be ignored by watches on "foo" and "foo/bar" with IN_EXCL_UNLINK. func (w *Watch) ExcludeUnlinked() bool { - return atomic.LoadUint32(&w.mask)&linux.IN_EXCL_UNLINK != 0 + return w.mask.Load()&linux.IN_EXCL_UNLINK != 0 } // Notify queues a new event on this watch. Returns true if this is a one-shot // watch that should be deleted, after this event was successfully queued. func (w *Watch) Notify(name string, events uint32, cookie uint32) bool { - if atomic.LoadInt32(&w.expired) == 1 { + if w.expired.Load() == 1 { // This is a one-shot watch that is already in the process of being // removed. This may happen if a second event reaches the watch target // before this watch has been removed. return false } - mask := atomic.LoadUint32(&w.mask) + mask := w.mask.Load() if mask&events == 0 { // We weren't watching for this event. return false @@ -610,7 +608,7 @@ func (w *Watch) Notify(name string, events uint32, cookie uint32) bool { matchedEvents := effectiveMask & events w.owner.queueEvent(newEvent(w.wd, name, matchedEvents, cookie)) if mask&linux.IN_ONESHOT != 0 { - atomic.StoreInt32(&w.expired, 1) + w.expired.Store(1) return true } return false diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 9a2bf2b0d..321e1c2d4 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -20,7 +20,6 @@ import ( "math" "sort" "strings" - "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -446,7 +445,7 @@ func (vfs *VirtualFilesystem) connectLocked(mnt *Mount, vd VirtualDentry, mntns vd.mount.children = make(map[*Mount]struct{}) } vd.mount.children[mnt] = struct{}{} - atomic.AddUint32(&vd.dentry.mounts, 1) + vd.dentry.mounts.Add(1) mnt.ns = mntns mntns.mountpoints[vd.dentry]++ vfs.mounts.insertSeqed(mnt) @@ -474,7 +473,7 @@ func (vfs *VirtualFilesystem) disconnectLocked(mnt *Mount) VirtualDentry { } mnt.loadKey(VirtualDentry{}) delete(vd.mount.children, mnt) - atomic.AddUint32(&vd.dentry.mounts, math.MaxUint32) // -1 + vd.dentry.mounts.Add(math.MaxUint32) // -1 mnt.ns.mountpoints[vd.dentry]-- if mnt.ns.mountpoints[vd.dentry] == 0 { delete(mnt.ns.mountpoints, vd.dentry) diff --git a/pkg/sleep/BUILD b/pkg/sleep/BUILD index 48bcdd62b..21df8994a 100644 --- a/pkg/sleep/BUILD +++ b/pkg/sleep/BUILD @@ -18,4 +18,5 @@ go_test( "sleep_test.go", ], library = ":sleep", + deps = ["//pkg/atomicbitops"], ) diff --git a/pkg/sleep/sleep_test.go b/pkg/sleep/sleep_test.go index 49fbd35a5..73360e79c 100644 --- a/pkg/sleep/sleep_test.go +++ b/pkg/sleep/sleep_test.go @@ -18,9 +18,10 @@ import ( "math/rand" "runtime" "sync" - "sync/atomic" "testing" "time" + + "gvisor.dev/gvisor/pkg/atomicbitops" ) // ZeroWakerNotAsserted tests that a zero-value waker is in non-asserted state. @@ -351,7 +352,7 @@ func TestAssertFetch(t *testing.T) { } }() var ( - count int32 + count atomicbitops.Int32 wg sync.WaitGroup ) for i := 0; i < sleeperWakers; i++ { @@ -361,7 +362,7 @@ func TestAssertFetch(t *testing.T) { ss[i].Fetch(true /* block */) w := &ws[(i+1)%sleeperWakers] for n := 0; n < wakeRequests; n++ { - atomic.AddInt32(&count, 1) + count.Add(1) ss[i].AssertAndFetch(w) } w.Assert() // Final wake-up. @@ -373,8 +374,8 @@ func TestAssertFetch(t *testing.T) { wg.Wait() // Check what we got. - if want := int32(sleeperWakers * wakeRequests); count != want { - t.Errorf("unexpected count: got %d, wanted %d", count, want) + if got, want := count.Load(), int32(sleeperWakers*wakeRequests); got != want { + t.Errorf("unexpected count: got %d, wanted %d", got, want) } } diff --git a/pkg/sync/gate_test.go b/pkg/sync/gate_test.go index 82ce02b97..37c74028e 100644 --- a/pkg/sync/gate_test.go +++ b/pkg/sync/gate_test.go @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +checkalignedignore package sync import ( diff --git a/pkg/sync/mutex_test.go b/pkg/sync/mutex_test.go index 9e4e3f0b2..4122b2e82 100644 --- a/pkg/sync/mutex_test.go +++ b/pkg/sync/mutex_test.go @@ -3,6 +3,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +// +checkalignedignore package sync import ( diff --git a/pkg/syncevent/BUILD b/pkg/syncevent/BUILD index 42c553308..a661baef7 100644 --- a/pkg/syncevent/BUILD +++ b/pkg/syncevent/BUILD @@ -28,6 +28,7 @@ go_test( ], library = ":syncevent", deps = [ + "//pkg/atomicbitops", "//pkg/sleep", "//pkg/sync", "//pkg/waiter", diff --git a/pkg/syncevent/syncevent_example_test.go b/pkg/syncevent/syncevent_example_test.go index bfb18e2ea..2f82b57ce 100644 --- a/pkg/syncevent/syncevent_example_test.go +++ b/pkg/syncevent/syncevent_example_test.go @@ -16,8 +16,9 @@ package syncevent import ( "fmt" - "sync/atomic" "time" + + "gvisor.dev/gvisor/pkg/atomicbitops" ) func Example_ioReadinessInterrputible() { @@ -30,10 +31,10 @@ func Example_ioReadinessInterrputible() { // State of some I/O object. var ( br Broadcaster - ready uint32 + ready atomicbitops.Uint32 ) doIO := func() error { - if atomic.LoadUint32(&ready) == 0 { + if ready.Load() == 0 { return errNotReady } return nil @@ -43,7 +44,7 @@ func Example_ioReadinessInterrputible() { time.Sleep(100 * time.Millisecond) // When it does, it first ensures that future calls to isReady() return // true, then broadcasts the readiness event to Receivers. - atomic.StoreUint32(&ready, 1) + ready.Store(1) br.Broadcast(evReady) }() diff --git a/pkg/syncevent/waiter_test.go b/pkg/syncevent/waiter_test.go index 428b20d0d..8bfeda13d 100644 --- a/pkg/syncevent/waiter_test.go +++ b/pkg/syncevent/waiter_test.go @@ -16,10 +16,10 @@ package syncevent import ( "fmt" - "sync/atomic" "testing" "time" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/sleep" "gvisor.dev/gvisor/pkg/sync" ) @@ -53,16 +53,16 @@ func TestWaiterWaitFor(t *testing.T) { evWaited := Set(1) evOther := Set(2) w.Notify(evOther) - notifiedEvent := uint32(0) + notifiedEvent := atomicbitops.FromUint32(0) go func() { time.Sleep(100 * time.Millisecond) - atomic.StoreUint32(¬ifiedEvent, 1) + notifiedEvent.Store(1) w.Notify(evWaited) }() if got, want := w.WaitFor(evWaited), evWaited|evOther; got != want { t.Errorf("Waiter.WaitFor: got %#x, wanted %#x", got, want) } - if atomic.LoadUint32(¬ifiedEvent) == 0 { + if notifiedEvent.Load() == 0 { t.Errorf("Waiter.WaitFor returned before goroutine notified waited-for event") } } diff --git a/pkg/unet/BUILD b/pkg/unet/BUILD index 8902be2d3..a78d3ebf8 100644 --- a/pkg/unet/BUILD +++ b/pkg/unet/BUILD @@ -10,6 +10,7 @@ go_library( ], visibility = ["//visibility:public"], deps = [ + "//pkg/atomicbitops", "//pkg/eventfd", "//pkg/sync", "@org_golang_x_sys//unix:go_default_library", diff --git a/pkg/unet/unet.go b/pkg/unet/unet.go index 0dc0c37bd..05d56538f 100644 --- a/pkg/unet/unet.go +++ b/pkg/unet/unet.go @@ -20,9 +20,9 @@ package unet import ( "errors" - "sync/atomic" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/eventfd" "gvisor.dev/gvisor/pkg/sync" ) @@ -63,9 +63,8 @@ type Socket struct { // fd is the bound socket. // - // fd must be read atomically, and only remains valid if read while - // within gate. - fd int32 + // fd only remains valid if read while within gate. + fd atomicbitops.Int32 // efd is an event FD that is signaled when the socket is closing. // @@ -74,7 +73,7 @@ type Socket struct { // race is an atomic variable used to avoid triggering the race // detector. See comment in SocketPair below. - race *int32 + race *atomicbitops.Int32 } // NewSocket returns a socket from an existing FD. @@ -93,7 +92,7 @@ func NewSocket(fd int) (*Socket, error) { } return &Socket{ - fd: int32(fd), + fd: atomicbitops.FromInt32(int32(fd)), efd: efd, }, nil } @@ -116,7 +115,7 @@ func (s *Socket) finish() error { func (s *Socket) Close() error { // Set the FD in the socket to -1, to ensure that all future calls to // FD/Release get nothing and Close calls return immediately. - fd := int(atomic.SwapInt32(&s.fd, -1)) + fd := int(s.fd.Swap(-1)) if fd < 0 { // Already closed or closing. return unix.EBADF @@ -140,7 +139,7 @@ func (s *Socket) Close() error { func (s *Socket) Release() (int, error) { // Set the FD in the socket to -1, to ensure that all future calls to // FD/Release get nothing and Close calls return immediately. - fd := int(atomic.SwapInt32(&s.fd, -1)) + fd := int(s.fd.Swap(-1)) if fd < 0 { // Already closed or closing. return -1, unix.EBADF @@ -165,7 +164,7 @@ func (s *Socket) Release() (int, error) { // // Use Release to take ownership of the FD. func (s *Socket) FD() int { - return int(atomic.LoadInt32(&s.fd)) + return int(s.fd.Load()) } // enterFD enters the FD gate and returns the FD value. @@ -179,7 +178,7 @@ func (s *Socket) enterFD() (int, bool) { return -1, false } - fd := int(atomic.LoadInt32(&s.fd)) + fd := int(s.fd.Load()) if fd < 0 { s.gate.Leave() return -1, false @@ -203,13 +202,13 @@ func SocketPair(packet bool) (*Socket, *Socket, error) { // // NOTE(b/27107811): This is purely due to the fact that the raw // syscall does not serve as a boundary for the sanitizer. - var race int32 a, err := NewSocket(fds[0]) if err != nil { unix.Close(fds[0]) unix.Close(fds[1]) return nil, nil, err } + var race atomicbitops.Int32 a.race = &race b, err := NewSocket(fds[1]) if err != nil { @@ -308,7 +307,7 @@ type SocketWriter struct { socket *Socket to []byte blocking bool - race *int32 + race *atomicbitops.Int32 ControlMessage } @@ -417,7 +416,7 @@ type SocketReader struct { socket *Socket source []byte blocking bool - race *int32 + race *atomicbitops.Int32 ControlMessage } diff --git a/pkg/unet/unet_unsafe.go b/pkg/unet/unet_unsafe.go index ea281fec3..1ac3824a5 100644 --- a/pkg/unet/unet_unsafe.go +++ b/pkg/unet/unet_unsafe.go @@ -16,7 +16,6 @@ package unet import ( "io" - "sync/atomic" "unsafe" "golang.org/x/sys/unix" @@ -30,7 +29,7 @@ func (s *Socket) wait(write bool) error { for { // Checking the FD on each loop is not strictly necessary, it // just avoids an extra poll call. - fd := atomic.LoadInt32(&s.fd) + fd := s.fd.Load() if fd < 0 { return errClosing } @@ -168,7 +167,7 @@ func (r *SocketReader) ReadVec(bufs [][]byte) (int, error) { if r.race != nil { // See comments on Socket.race. - atomic.AddInt32(r.race, 1) + r.race.Add(1) } if int(n) > length { @@ -187,7 +186,7 @@ func (w *SocketWriter) WriteVec(bufs [][]byte) (int, error) { if w.race != nil { // See comments on Socket.race. - atomic.AddInt32(w.race, 1) + w.race.Add(1) } var msg unix.Msghdr diff --git a/pkg/waiter/BUILD b/pkg/waiter/BUILD index a3251cdec..505d3eef5 100644 --- a/pkg/waiter/BUILD +++ b/pkg/waiter/BUILD @@ -23,6 +23,7 @@ go_library( ], visibility = ["//visibility:public"], deps = [ + "//pkg/atomicbitops", "//pkg/sync", ], ) @@ -34,4 +35,5 @@ go_test( "waiter_test.go", ], library = ":waiter", + deps = ["//pkg/atomicbitops"], ) diff --git a/pkg/waiter/waiter_test.go b/pkg/waiter/waiter_test.go index dbd127aa0..034db3ea3 100644 --- a/pkg/waiter/waiter_test.go +++ b/pkg/waiter/waiter_test.go @@ -15,8 +15,9 @@ package waiter import ( - "sync/atomic" "testing" + + "gvisor.dev/gvisor/pkg/atomicbitops" ) func TestEmptyQueue(t *testing.T) { @@ -141,14 +142,14 @@ func TestConcurrentRegistration(t *testing.T) { func TestConcurrentNotification(t *testing.T) { var q Queue - var cnt int32 + var cnt atomicbitops.Int32 const concurrency = 1000 const waiterCount = 1000 // Register waiters. for i := 0; i < waiterCount; i++ { e := NewFunctionEntry(EventIn|EventErr, func(mask EventMask) { - atomic.AddInt32(&cnt, 1) + cnt.Add(1) if mask != EventIn { t.Errorf("mask = %#x want %#x", mask, EventIn) } @@ -175,7 +176,7 @@ func TestConcurrentNotification(t *testing.T) { } // Check the count. - if cnt != concurrency*waiterCount { - t.Errorf("cnt = %d, want %d", cnt, concurrency*waiterCount) + if cnt.Load() != concurrency*waiterCount { + t.Errorf("cnt = %d, want %d", cnt.Load(), concurrency*waiterCount) } } diff --git a/runsc/fsgofer/BUILD b/runsc/fsgofer/BUILD index 8d5a6d300..a7ac16bac 100644 --- a/runsc/fsgofer/BUILD +++ b/runsc/fsgofer/BUILD @@ -14,6 +14,7 @@ go_library( visibility = ["//runsc:__subpackages__"], deps = [ "//pkg/abi/linux", + "//pkg/atomicbitops", "//pkg/cleanup", "//pkg/fd", "//pkg/lisafs", diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index e0d57d53d..e931af069 100644 --- a/runsc/fsgofer/lisafs.go +++ b/runsc/fsgofer/lisafs.go @@ -19,10 +19,10 @@ import ( "math" "path" "strconv" - "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/cleanup" rwfd "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/lisafs" @@ -66,7 +66,7 @@ func (s *LisafsServer) Mount(c *lisafs.Connection, mountNode *lisafs.Node) (*lis rootFD := &controlFDLisa{ hostFD: rootHostFD, - writableHostFD: -1, + writableHostFD: atomicbitops.FromInt32(-1), } mountNode.IncRef() // Ref is transferred to ControlFD. rootFD.ControlFD.Init(c, mountNode, linux.FileMode(stat.Mode), rootFD) @@ -117,10 +117,10 @@ type controlFDLisa struct { // hostFD is the file descriptor which can be used to make host syscalls. hostFD int - // writableHostFD is the file descriptor number for a writable FD opened on the - // same FD as `hostFD`. writableHostFD must only be accessed using atomic - // operations. It is initialized to -1, and can change in value exactly once. - writableHostFD int32 + // writableHostFD is the file descriptor number for a writable FD opened on + // the same FD as `hostFD`. It is initialized to -1, and can change in value + // exactly once. + writableHostFD atomicbitops.Int32 } var _ lisafs.ControlFDImpl = (*controlFDLisa)(nil) @@ -152,13 +152,13 @@ func newControlFDLisa(hostFD int, parent *controlFDLisa, name string, mode linux } }) childFD.hostFD = hostFD - childFD.writableHostFD = -1 + childFD.writableHostFD = atomicbitops.FromInt32(-1) childFD.ControlFD.Init(parent.Conn(), childNode, mode, childFD) return childFD } func (fd *controlFDLisa) getWritableFD() (int, error) { - if writableFD := atomic.LoadInt32(&fd.writableHostFD); writableFD != -1 { + if writableFD := fd.writableHostFD.Load(); writableFD != -1 { return int(writableFD), nil } @@ -166,10 +166,10 @@ func (fd *controlFDLisa) getWritableFD() (int, error) { if err != nil { return -1, err } - if !atomic.CompareAndSwapInt32(&fd.writableHostFD, -1, int32(writableFD)) { + if !fd.writableHostFD.CompareAndSwap(-1, int32(writableFD)) { // Race detected, use the new value and clean this up. unix.Close(writableFD) - return int(atomic.LoadInt32(&fd.writableHostFD)), nil + return int(fd.writableHostFD.Load()), nil } return writableFD, nil } @@ -189,9 +189,9 @@ func (fd *controlFDLisa) Close() { fd.hostFD = -1 } // No concurrent access is possible so no need to use atomics. - if fd.writableHostFD >= 0 { - _ = unix.Close(int(fd.writableHostFD)) - fd.writableHostFD = -1 + if fd.writableHostFD.RacyLoad() >= 0 { + _ = unix.Close(int(fd.writableHostFD.RacyLoad())) + fd.writableHostFD = atomicbitops.FromInt32(-1) } } diff --git a/tools/checklocks/BUILD b/tools/checklocks/BUILD index 4bf918f48..e0488664f 100644 --- a/tools/checklocks/BUILD +++ b/tools/checklocks/BUILD @@ -18,6 +18,7 @@ go_library( "//tools/nogo:__subpackages__", ], deps = [ + "//pkg/atomicbitops", "@org_golang_x_tools//go/analysis:go_default_library", "@org_golang_x_tools//go/analysis/passes/buildssa:go_default_library", "@org_golang_x_tools//go/ssa:go_default_library", diff --git a/tools/checklocks/state.go b/tools/checklocks/state.go index 2de373b27..72d9c2b0c 100644 --- a/tools/checklocks/state.go +++ b/tools/checklocks/state.go @@ -19,9 +19,9 @@ import ( "go/token" "go/types" "strings" - "sync/atomic" "golang.org/x/tools/go/ssa" + "gvisor.dev/gvisor/pkg/atomicbitops" ) // lockInfo describes a held lock. @@ -52,12 +52,12 @@ type lockState struct { // refs indicates the number of references on this structure. If it's // greater than one, we will do copy-on-write. - refs *int32 + refs *atomicbitops.Int32 } // newLockState makes a new lockState. func newLockState() *lockState { - refs := int32(1) // Not shared. + refs := atomicbitops.FromInt32(1) // Not shared. return &lockState{ lockedMutexes: make(map[string]lockInfo), used: make(map[ssa.Value]struct{}), @@ -73,7 +73,7 @@ func (l *lockState) fork() *lockState { if l == nil { return newLockState() } - atomic.AddInt32(l.refs, 1) + l.refs.Add(1) return &lockState{ lockedMutexes: l.lockedMutexes, used: make(map[ssa.Value]struct{}), @@ -85,7 +85,7 @@ func (l *lockState) fork() *lockState { // modify indicates that this state will be modified. func (l *lockState) modify() { - if atomic.LoadInt32(l.refs) > 1 { + if l.refs.Load() > 1 { // Copy the lockedMutexes. lm := make(map[string]lockInfo) for k, v := range l.lockedMutexes { @@ -109,8 +109,8 @@ func (l *lockState) modify() { l.defers = ds // Drop our reference. - atomic.AddInt32(l.refs, -1) - newRefs := int32(1) // Not shared. + l.refs.Add(-1) + newRefs := atomicbitops.FromInt32(1) // Not shared. l.refs = &newRefs } } diff --git a/tools/checklocks/test/test.go b/tools/checklocks/test/test.go index d1a9992fb..fd2a92162 100644 --- a/tools/checklocks/test/test.go +++ b/tools/checklocks/test/test.go @@ -15,6 +15,8 @@ // Package test is a test package. // // Tests are all compilation tests in separate files. +// +// +checkalignedignore package test import ( diff --git a/tools/go_fieldenum/main.go b/tools/go_fieldenum/main.go index d801bea1b..0ec7f471a 100644 --- a/tools/go_fieldenum/main.go +++ b/tools/go_fieldenum/main.go @@ -89,7 +89,7 @@ func main() { // Collect information for each type for which code is being generated. structInfos := make([]structInfo, 0, len(typeNames)) - needSyncAtomic := false + needAtomic := false for _, typeName := range typeNames { typeInfo := fieldEnumTypes[typeName] var si structInfo @@ -120,8 +120,8 @@ func main() { si.allFields = append(si.allFields, fieldSetField{ fieldName: name, }) - // sync/atomic import will be needed for FieldSet.Load(). - needSyncAtomic = true + // atomicbitops import will be needed for FieldSet.Load(). + needAtomic = true } structInfos = append(structInfos, si) } @@ -130,8 +130,9 @@ func main() { var b strings.Builder fmt.Fprintf(&b, "// Generated by go_fieldenum.\n\n") fmt.Fprintf(&b, "package %s\n\n", *outputPkg) - if needSyncAtomic { - fmt.Fprintf(&b, "import \"sync/atomic\"\n\n") + if needAtomic { + fmt.Fprintf(&b, `import "gvisor.dev/gvisor/pkg/atomicbitops"`) + fmt.Fprintf(&b, "\n\n") } for _, si := range structInfos { si.writeTo(&b) @@ -240,35 +241,35 @@ func (si *structInfo) writeTo(b *strings.Builder) { fmt.Fprintf(b, "\t%s %sFieldSet\n", fieldSetField.fieldName, fieldSetField.typePrefix) } if len(si.reprByBit) != 0 { - fmt.Fprintf(b, "\tfields [%d]uint32\n", numBitmaskUint32s) + fmt.Fprintf(b, "\tfields [%d]atomicbitops.Uint32\n", numBitmaskUint32s) } fmt.Fprintf(b, "}\n\n") if len(si.reprByBit) != 0 { fmt.Fprintf(b, "// Contains returns true if f is present in the %sFieldSet.\n", si.prefix) - fmt.Fprintf(b, "func (fs %sFieldSet) Contains(f %sField) bool {\n", si.prefix, si.prefix) + fmt.Fprintf(b, "func (fs *%sFieldSet) Contains(f %sField) bool {\n", si.prefix, si.prefix) if numBitmaskUint32s == 1 { - fmt.Fprintf(b, "\treturn fs.fields[0] & (uint32(1) << uint(f)) != 0\n") + fmt.Fprintf(b, "\treturn fs.fields[0].RacyLoad() & (uint32(1) << uint(f)) != 0\n") } else { - fmt.Fprintf(b, "\treturn fs.fields[f/32] & (uint32(1) << (f%%32)) != 0\n") + fmt.Fprintf(b, "\treturn fs.fields[f/32].RacyLoad() & (uint32(1) << (f%%32)) != 0\n") } fmt.Fprintf(b, "}\n\n") fmt.Fprintf(b, "// Add adds f to the %sFieldSet.\n", si.prefix) fmt.Fprintf(b, "func (fs *%sFieldSet) Add(f %sField) {\n", si.prefix, si.prefix) if numBitmaskUint32s == 1 { - fmt.Fprintf(b, "\tfs.fields[0] |= uint32(1) << uint(f)\n") + fmt.Fprintf(b, "\tfs.fields[0] = atomicbitops.FromUint32(fs.fields[0].RacyLoad() | (uint32(1) << uint(f)))\n") } else { - fmt.Fprintf(b, "\tfs.fields[f/32] |= uint32(1) << (f%%32)\n") + fmt.Fprintf(b, "\tfs.fields[f/32] = atomicbitops.FromUint32(fs.fields[f/32].RacyLoad() | (uint32(1) << (f%%32))\n") } fmt.Fprintf(b, "}\n\n") fmt.Fprintf(b, "// Remove removes f from the %sFieldSet.\n", si.prefix) fmt.Fprintf(b, "func (fs *%sFieldSet) Remove(f %sField) {\n", si.prefix, si.prefix) if numBitmaskUint32s == 1 { - fmt.Fprintf(b, "\tfs.fields[0] &^= uint32(1) << uint(f)\n") + fmt.Fprintf(b, "\tfs.fields[0] = atomicbitops.FromUint32(fs.fields[0].RacyLoad() &^ (uint32(1) << uint(f)))\n") } else { - fmt.Fprintf(b, "\tfs.fields[f/32] &^= uint32(1) << (f%%32)\n") + fmt.Fprintf(b, "\tfs.fields[f/32] = atomicbitops.FromUint32(fs.fields[f/32].RacyLoad() &^ (uint32(1) << uint(f%%32)))\n") } fmt.Fprintf(b, "}\n\n") } @@ -280,7 +281,7 @@ func (si *structInfo) writeTo(b *strings.Builder) { fmt.Fprintf(b, "\tcopied.%s = fs.%s.Load()\n", fieldSetField.fieldName, fieldSetField.fieldName) } for i := 0; i < numBitmaskUint32s; i++ { - fmt.Fprintf(b, "\tcopied.fields[%d] = atomic.LoadUint32(&fs.fields[%d])\n", i, i) + fmt.Fprintf(b, "\tcopied.fields[%d] = atomicbitops.FromUint32(fs.fields[%d].Load())\n", i, i) } fmt.Fprintf(b, "\treturn\n") fmt.Fprintf(b, "}\n\n") @@ -295,10 +296,10 @@ func (si *structInfo) writeTo(b *strings.Builder) { fieldConstName := fmt.Sprintf("%sField%s", si.prefix, fieldName) fmt.Fprintf(b, "\tif fields.%s {\n", fieldName) if numBitmaskUint32s == 1 { - fmt.Fprintf(b, "\t\tatomic.StoreUint32(&fs.fields[0], fs.fields[0] | (uint32(1) << uint(%s)))\n", fieldConstName) + fmt.Fprintf(b, "\t\tfs.fields[0].Store(fs.fields[0].RacyLoad() | (uint32(1) << uint(%s)))\n", fieldConstName) } else { fmt.Fprintf(b, "\t\tword, bit := %s/32, %s%%32\n", fieldConstName, fieldConstName) - fmt.Fprintf(b, "\t\tatomic.StoreUint32(&fs.fields[word], fs.fields[word] | (uint32(1) << bit))\n") + fmt.Fprintf(b, "\t\tfs.fields[word].Store(fs.fields[word].RacyLoad() | (uint32(1) << bit))\n") } fmt.Fprintf(b, "\t}\n") }