From 5efe17fe74923dcd0646ee00100f9c116c76d89a Mon Sep 17 00:00:00 2001 From: Deepak Jois Date: Fri, 10 Nov 2017 10:30:46 +0530 Subject: [PATCH] Fixing 64-bit alignment in structs that use sync.atomic This change fixes segmentation faults on Arm v7. From https://golang.org/pkg/sync/atomic/: > On both ARM and x86-32, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically. The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned. Had to modify a couple of tests to get them to pass. Fixes #311. --- db_test.go | 4 ++-- levels.go | 5 ++--- skl/skl.go | 26 ++++++++++++++++--------- transaction.go | 8 ++++---- transaction_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 18 deletions(-) diff --git a/db_test.go b/db_test.go index faa5c57d2..dee2a8440 100644 --- a/db_test.go +++ b/db_test.go @@ -328,7 +328,7 @@ func TestGetMore(t *testing.T) { } // n := 500000 n := 10000 - m := 49 // Increasing would cause ErrTxnTooBig + m := 45 // Increasing would cause ErrTxnTooBig for i := 0; i < n; i += m { txn := kv.NewTransaction(true) for j := i; j < i+m && j < n; j++ { @@ -432,7 +432,7 @@ func TestExistsMore(t *testing.T) { // n := 500000 n := 10000 - m := 49 + m := 45 for i := 0; i < n; i += m { if (i % 1000) == 0 { t.Logf("Putting i=%d\n", i) diff --git a/levels.go b/levels.go index 477666b45..5941a4161 100644 --- a/levels.go +++ b/levels.go @@ -32,14 +32,13 @@ import ( ) type levelsController struct { - elog trace.EventLog + nextFileID uint64 // Atomic + elog trace.EventLog // The following are initialized once and const. levels []*levelHandler kv *DB - nextFileID uint64 // Atomic - cstatus compactStatus } diff --git a/skl/skl.go b/skl/skl.go index eb0673fe4..7361751a5 100644 --- a/skl/skl.go +++ b/skl/skl.go @@ -50,6 +50,13 @@ const ( const MaxNodeSize = int(unsafe.Sizeof(node{})) type node struct { + // Multiple parts of the value are encoded as a single uint64 so that it + // can be atomically loaded and stored: + // value offset: uint32 (bits 0-31) + // value size : uint16 (bits 32-47) + // 12 bytes are allocated to ensure 8 byte alignment also on 32bit systems. + value [12]byte + // A byte slice is 24 bytes. We are trying to save space here. keyOffset uint32 // Immutable. No need to lock to access key. keySize uint16 // Immutable. No need to lock to access key. @@ -57,12 +64,6 @@ type node struct { // Height of the tower. height uint16 - // Multiple parts of the value are encoded as a single uint64 so that it - // can be atomically loaded and stored: - // value offset: uint32 (bits 0-31) - // value size : uint16 (bits 32-47) - value uint64 - // Most nodes do not need to use the full height of the tower, since the // probability of each successive level decreases exponentially. Because // these elements are never accessed, they do not need to be allocated. @@ -108,7 +109,7 @@ func newNode(arena *Arena, key []byte, v y.ValueStruct, height int) *node { node.keyOffset = arena.putKey(key) node.keySize = uint16(len(key)) node.height = uint16(height) - node.value = encodeValue(arena.putVal(v), v.EncodedSize()) + *node.value64BitAlignedPtr() = encodeValue(arena.putVal(v), v.EncodedSize()) return node } @@ -134,8 +135,15 @@ func NewSkiplist(arenaSize int64) *Skiplist { } } +func (s *node) value64BitAlignedPtr() *uint64 { + if uintptr(unsafe.Pointer(&s.value))%8 == 0 { + return (*uint64)(unsafe.Pointer(&s.value)) + } + return (*uint64)(unsafe.Pointer(&s.value[4])) +} + func (s *node) getValueOffset() (uint32, uint16) { - value := atomic.LoadUint64(&s.value) + value := atomic.LoadUint64(s.value64BitAlignedPtr()) return decodeValue(value) } @@ -146,7 +154,7 @@ func (s *node) key(arena *Arena) []byte { func (s *node) setValue(arena *Arena, v y.ValueStruct) { valOffset := arena.putVal(v) value := encodeValue(valOffset, v.EncodedSize()) - atomic.StoreUint64(&s.value, value) + atomic.StoreUint64(s.value64BitAlignedPtr(), value) } func (s *node) getNextOffset(h int) uint32 { diff --git a/transaction.go b/transaction.go index b330cc009..c1da2413e 100644 --- a/transaction.go +++ b/transaction.go @@ -46,10 +46,11 @@ func (u *uint64Heap) Pop() interface{} { } type oracle struct { + curRead uint64 // Managed by the mutex. + refCount int64 isManaged bool // Does not change value, so no locking required. sync.Mutex - curRead uint64 nextCommit uint64 // These two structures are used to figure out when a commit is done. The minimum done commit is @@ -59,8 +60,7 @@ type oracle struct { // commits stores a key fingerprint and latest commit counter for it. // refCount is used to clear out commits map to avoid a memory blowup. - commits map[uint64]uint64 - refCount int64 + commits map[uint64]uint64 } func (o *oracle) addRef() { @@ -192,7 +192,7 @@ type Txn struct { func (txn *Txn) checkSize(e *entry) error { count := txn.count + 1 // Extra bytes for version in key. - size := txn.size + int64(e.estimateSize(txn.db.opt.ValueThreshold)) + 10 + size := txn.size + int64(e.estimateSize(txn.db.opt.ValueThreshold)) + 10 if count >= txn.db.opt.maxBatchCount || size >= txn.db.opt.maxBatchSize { return ErrTxnTooBig } diff --git a/transaction_test.go b/transaction_test.go index dc4a775fd..1275c889a 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -23,6 +23,7 @@ import ( "strconv" "testing" + "github.com/dgraph-io/badger/options" "github.com/dgraph-io/badger/y" "github.com/stretchr/testify/require" @@ -617,3 +618,49 @@ func TestManagedDB(t *testing.T) { } txn.Discard() } + +func TestArmV7Issue311Fix(t *testing.T) { + dir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + config := DefaultOptions + config.TableLoadingMode = options.MemoryMap + config.ValueLogFileSize = 16 << 20 + config.LevelOneSize = 8 << 20 + config.MaxTableSize = 2 << 20 + config.Dir = dir + config.ValueDir = dir + config.SyncWrites = false + + db, err := Open(config) + if err != nil { + t.Fatalf("cannot open db at location %s: %v", dir, err) + } + + err = db.View(func(txn *Txn) error { return nil }) + if err != nil { + t.Fatal(err) + } + + err = db.Update(func(txn *Txn) error { + return txn.Set([]byte{0x11}, []byte{0x22}) + }) + if err != nil { + t.Fatal(err) + } + + err = db.Update(func(txn *Txn) error { + return txn.Set([]byte{0x11}, []byte{0x22}) + }) + + if err != nil { + t.Fatal(err) + } + + if err = db.Close(); err != nil { + t.Fatal(err) + } +}