From 9dcf6732164003a8a66d69b23f6d10414dedb621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Uwe=20Kr=C3=BCger?= <45282134+UweKrueger@users.noreply.github.com> Date: Wed, 3 Feb 2021 15:16:52 +0100 Subject: [PATCH] all: make `lock` and `rlock` dead lock free :-) (#8534) --- vlib/v/ast/ast.v | 2 +- vlib/v/checker/checker.v | 23 +++++-- vlib/v/checker/tests/lock_already_locked.out | 4 +- vlib/v/checker/tests/lock_already_rlocked.out | 4 +- vlib/v/checker/tests/shared_bad_args.out | 4 +- vlib/v/fmt/fmt.v | 41 +++++++++-- vlib/v/gen/c/cgen.v | 48 ++++++++++--- vlib/v/gen/c/cheaders.v | 15 ++++ vlib/v/parser/lock.v | 45 ++++++++---- vlib/v/tests/shared_lock_5_test.v | 68 +++++++++++++++++++ vlib/v/tests/shared_lock_6_test.v | 59 ++++++++++++++++ vlib/v/tests/shared_unordered_mixed_test.v | 66 ++++++++++++++++++ 12 files changed, 338 insertions(+), 41 deletions(-) create mode 100644 vlib/v/tests/shared_lock_5_test.v create mode 100644 vlib/v/tests/shared_lock_6_test.v create mode 100644 vlib/v/tests/shared_unordered_mixed_test.v diff --git a/vlib/v/ast/ast.v b/vlib/v/ast/ast.v index f77b33d4c2..a7cd74cacb 100644 --- a/vlib/v/ast/ast.v +++ b/vlib/v/ast/ast.v @@ -646,7 +646,7 @@ pub: pub struct LockExpr { pub: stmts []Stmt - is_rlock bool + is_rlock []bool pos token.Position pub mut: lockeds []Ident // `x`, `y` in `lock x, y {` diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index c67fb2c683..33960ffd75 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -1081,6 +1081,15 @@ fn (mut c Checker) fail_if_immutable(expr ast.Expr) (string, token.Position) { v.is_changed = true if v.typ.share() == .shared_t { if expr.name !in c.locked_names { + if c.locked_names.len > 0 || c.rlocked_names.len > 0 { + if expr.name in c.rlocked_names { + c.error('$expr.name has an `rlock` but needs a `lock`', + expr.pos) + } else { + c.error('$expr.name must be added to the `lock` list above', + expr.pos) + } + } to_lock = expr.name pos = expr.pos } @@ -2620,6 +2629,8 @@ pub fn (mut c Checker) assign_stmt(mut assign_stmt ast.AssignStmt) { } left_type = left_type.set_nr_muls(1) } + } else if left_type.has_flag(.shared_f) { + left_type = left_type.clear_flag(.shared_f) } if ident_var_info.share == .atomic_t { left_type = left_type.set_flag(.atomic_f) @@ -4540,6 +4551,9 @@ pub fn (mut c Checker) select_expr(mut node ast.SelectExpr) table.Type { } pub fn (mut c Checker) lock_expr(mut node ast.LockExpr) table.Type { + if c.rlocked_names.len > 0 || c.locked_names.len > 0 { + c.error('nested `lock`/`rlock` not allowed', node.pos) + } for i in 0 .. node.lockeds.len { c.ident(mut node.lockeds[i]) id := node.lockeds[i] @@ -4555,18 +4569,15 @@ pub fn (mut c Checker) lock_expr(mut node ast.LockExpr) table.Type { } else if id.name in c.rlocked_names { c.error('`$id.name` is already read-locked', id.pos) } - if node.is_rlock { + if node.is_rlock[i] { c.rlocked_names << id.name } else { c.locked_names << id.name } } c.stmts(node.stmts) - if node.is_rlock { - c.rlocked_names = c.rlocked_names[..c.rlocked_names.len - node.lockeds.len] - } else { - c.locked_names = c.locked_names[..c.locked_names.len - node.lockeds.len] - } + c.rlocked_names = [] + c.locked_names = [] // void for now... maybe sometime `x := lock a { a.getval() }` return table.void_type } diff --git a/vlib/v/checker/tests/lock_already_locked.out b/vlib/v/checker/tests/lock_already_locked.out index 65e6853964..7e421a1bcf 100644 --- a/vlib/v/checker/tests/lock_already_locked.out +++ b/vlib/v/checker/tests/lock_already_locked.out @@ -1,7 +1,7 @@ -vlib/v/checker/tests/lock_already_locked.vv:11:9: error: `a` is already locked +vlib/v/checker/tests/lock_already_locked.vv:11:3: error: nested `lock`/`rlock` not allowed 9 | } 10 | lock a { 11 | rlock a { - | ^ + | ~~~~~ 12 | a.x++ 13 | } diff --git a/vlib/v/checker/tests/lock_already_rlocked.out b/vlib/v/checker/tests/lock_already_rlocked.out index dc93e364cf..411907d912 100644 --- a/vlib/v/checker/tests/lock_already_rlocked.out +++ b/vlib/v/checker/tests/lock_already_rlocked.out @@ -1,7 +1,7 @@ -vlib/v/checker/tests/lock_already_rlocked.vv:11:8: error: `a` is already read-locked +vlib/v/checker/tests/lock_already_rlocked.vv:11:3: error: nested `lock`/`rlock` not allowed 9 | } 10 | rlock a { 11 | lock a { - | ^ + | ~~~~ 12 | a.x++ 13 | } diff --git a/vlib/v/checker/tests/shared_bad_args.out b/vlib/v/checker/tests/shared_bad_args.out index 2030b813ec..2a0708b5ae 100644 --- a/vlib/v/checker/tests/shared_bad_args.out +++ b/vlib/v/checker/tests/shared_bad_args.out @@ -26,14 +26,14 @@ vlib/v/checker/tests/shared_bad_args.vv:51:13: error: a is `shared` and must be | ^ 52 | println('$w $x') 53 | } -vlib/v/checker/tests/shared_bad_args.vv:61:3: error: r is `shared` and must be `lock`ed to be passed as `mut` +vlib/v/checker/tests/shared_bad_args.vv:61:3: error: r must be added to the `lock` list above 59 | shared r := Qr{ a: 7 } 60 | lock s { 61 | r.s_mut(mut s) | ^ 62 | } 63 | lock r { -vlib/v/checker/tests/shared_bad_args.vv:64:15: error: s is `shared` and must be `lock`ed to be passed as `mut` +vlib/v/checker/tests/shared_bad_args.vv:64:15: error: s must be added to the `lock` list above 62 | } 63 | lock r { 64 | r.s_mut(mut s) diff --git a/vlib/v/fmt/fmt.v b/vlib/v/fmt/fmt.v index 9407aa48bf..ff0b403630 100644 --- a/vlib/v/fmt/fmt.v +++ b/vlib/v/fmt/fmt.v @@ -1480,12 +1480,43 @@ pub fn (mut f Fmt) array_decompose(node ast.ArrayDecompose) { } pub fn (mut f Fmt) lock_expr(lex ast.LockExpr) { - f.write(if lex.is_rlock { 'rlock ' } else { 'lock ' }) - for i, v in lex.lockeds { - if i > 0 { - f.write(', ') + mut num_locked := 0 + mut num_rlocked := 0 + for is_rlock in lex.is_rlock { + if is_rlock { + num_rlocked++ + } else { + num_locked++ + } + } + if num_locked > 0 || num_rlocked == 0 { + f.write('lock ') + mut n := 0 + for i, v in lex.lockeds { + if !lex.is_rlock[i] { + if n > 0 { + f.write(', ') + } + f.expr(v) + n++ + } + } + } + if num_rlocked > 0 { + if num_locked > 0 { + f.write('; ') + } + f.write('rlock ') + mut n := 0 + for i, v in lex.lockeds { + if lex.is_rlock[i] { + if n > 0 { + f.write(', ') + } + f.expr(v) + n++ + } } - f.expr(v) } f.write(' {') f.writeln('') diff --git a/vlib/v/gen/c/cgen.v b/vlib/v/gen/c/cgen.v index 9fd0e14e76..a14eabb2f3 100644 --- a/vlib/v/gen/c/cgen.v +++ b/vlib/v/gen/c/cgen.v @@ -3405,22 +3405,54 @@ fn (mut g Gen) infix_expr(node ast.InfixExpr) { } fn (mut g Gen) lock_expr(node ast.LockExpr) { - mut lock_prefixes := []string{len: 0, cap: node.lockeds.len} - for id in node.lockeds { + mut mtxs := '' + if node.lockeds.len == 0 { + // this should not happen + } else if node.lockeds.len == 1 { + id := node.lockeds[0] name := id.name deref := if id.is_mut { '->' } else { '.' } - lock_prefix := if node.is_rlock { 'r' } else { '' } - lock_prefixes << lock_prefix // keep for unlock + lock_prefix := if node.is_rlock[0] { 'r' } else { '' } g.writeln('sync__RwMutex_${lock_prefix}lock(&$name${deref}mtx);') + } else { + mtxs = g.new_tmp_var() + g.writeln('uintptr_t _arr_$mtxs[$node.lockeds.len];') + g.writeln('bool _isrlck_$mtxs[$node.lockeds.len];') + for i, id in node.lockeds { + name := id.name + deref := if id.is_mut { '->' } else { '.' } + g.writeln('_arr_$mtxs[$i] = &$name${deref}mtx;') + // TODO: fix `vfmt` to allow this in string interpolation + is_rlock_str := node.is_rlock[i].str() + g.writeln('_isrlck_$mtxs[$i] = $is_rlock_str;') + } + g.writeln('__sort_ptr(_arr_$mtxs, _isrlck_$mtxs, $node.lockeds.len);') + g.writeln('for (int $mtxs=0; $mtxs<$node.lockeds.len; $mtxs++) {') + g.writeln('\tif ($mtxs && _arr_$mtxs[$mtxs] == _arr_$mtxs[$mtxs-1]) continue;') + g.writeln('\tif (_isrlck_$mtxs[$mtxs])') + g.writeln('\t\tsync__RwMutex_rlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);') + g.writeln('\telse') + g.writeln('\t\tsync__RwMutex_lock((sync__RwMutex*)_arr_$mtxs[$mtxs]);') + g.writeln('}') } g.stmts(node.stmts) - // unlock in reverse order - for i := node.lockeds.len - 1; i >= 0; i-- { - id := node.lockeds[i] - lock_prefix := lock_prefixes[i] + if node.lockeds.len == 0 { + // this should not happen + } else if node.lockeds.len == 1 { + id := node.lockeds[0] name := id.name deref := if id.is_mut { '->' } else { '.' } + lock_prefix := if node.is_rlock[0] { 'r' } else { '' } g.writeln('sync__RwMutex_${lock_prefix}unlock(&$name${deref}mtx);') + } else { + // unlock in reverse order + g.writeln('for (int $mtxs=${node.lockeds.len - 1}; $mtxs>=0; $mtxs--) {') + g.writeln('\tif ($mtxs && _arr_$mtxs[$mtxs] == _arr_$mtxs[$mtxs-1]) continue;') + g.writeln('\tif (_isrlck_$mtxs[$mtxs])') + g.writeln('\t\tsync__RwMutex_runlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);') + g.writeln('\telse') + g.writeln('\t\tsync__RwMutex_unlock((sync__RwMutex*)_arr_$mtxs[$mtxs]);') + g.writeln('}') } } diff --git a/vlib/v/gen/c/cheaders.v b/vlib/v/gen/c/cheaders.v index 1bf6b1712f..e0ae530983 100644 --- a/vlib/v/gen/c/cheaders.v +++ b/vlib/v/gen/c/cheaders.v @@ -31,6 +31,21 @@ static inline voidptr __dup_shared_array(voidptr src, int sz) { sync__RwMutex_init(&dest->mtx); return dest; } +static inline void __sort_ptr(uintptr_t a[], bool b[], int l) +{ + for (int i=1; i0 && (a[j-1] > ins || b[j-1] && !insb)) { + a[j] = a[j-1]; + b[j] = b[j-1]; + j--; + } + a[j] = ins; + b[j] = insb; + } +} ' c_common_macros = ' #define EMPTY_VARG_INITIALIZATION 0 diff --git a/vlib/v/parser/lock.v b/vlib/v/parser/lock.v index c69d6463ce..f3ef1c82ea 100644 --- a/vlib/v/parser/lock.v +++ b/vlib/v/parser/lock.v @@ -7,31 +7,46 @@ fn (mut p Parser) lock_expr() ast.LockExpr { // TODO Handle aliasing sync p.register_auto_import('sync') mut pos := p.tok.position() - is_rlock := p.tok.kind == .key_rlock - p.next() mut lockeds := []ast.Ident{} - for p.tok.kind == .name { - lockeds << ast.Ident{ - language: table.Language.v - pos: p.tok.position() - mod: p.mod - name: p.tok.lit - is_mut: true - info: ast.IdentVar{} - scope: p.scope + mut is_rlocked := []bool{} + for { + if p.tok.kind == .lcbr { + goto start_stmts + } + is_rlock := p.tok.kind == .key_rlock + if !is_rlock && p.tok.kind != .key_lock { + p.error_with_pos('unexpected `$p.tok.lit`; `lock` or `rlock` expected', p.tok.position()) } p.next() - if p.tok.kind == .lcbr { - break + for p.tok.kind == .name { + lockeds << ast.Ident{ + language: table.Language.v + pos: p.tok.position() + mod: p.mod + name: p.tok.lit + is_mut: true + info: ast.IdentVar{} + scope: p.scope + } + is_rlocked << is_rlock + p.next() + if p.tok.kind == .lcbr { + goto start_stmts + } + if p.tok.kind == .semicolon { + p.next() + break + } + p.check(.comma) } - p.check(.comma) } + start_stmts: stmts := p.parse_block() pos.update_last_line(p.prev_tok.line_nr) return ast.LockExpr{ lockeds: lockeds stmts: stmts - is_rlock: is_rlock + is_rlock: is_rlocked pos: pos } } diff --git a/vlib/v/tests/shared_lock_5_test.v b/vlib/v/tests/shared_lock_5_test.v new file mode 100644 index 0000000000..8aee4b6559 --- /dev/null +++ b/vlib/v/tests/shared_lock_5_test.v @@ -0,0 +1,68 @@ +struct St { +mut: + a int +} + +fn (shared x St) f(shared y St, shared z St) { + for _ in 0 .. 10000 { + lock x, y, z { + tmp := z.a + z.a = y.a + y.a = x.a + x.a = tmp + } + } +} + +fn (shared x St) g(shared y St, shared z St) { + for _ in 0 .. 10000 { + lock z, x, y { + tmp := x.a + x.a = z.a + z.a = y.a + y.a = tmp + } + } +} + +fn h(shared x St, shared y St, shared z St) { + for _ in 0 .. 10000 { + lock y, x, z { + tmp := y.a + y.a = z.a + z.a = x.a + x.a = tmp + } + } +} + +fn test_shared_receiver_lock() { + shared x := &St{ + a: 5 + } + shared y := &St{ + a: 7 + } + shared z := &St{ + a: 1 + } + t1 := go x.f(shared y, shared z) + t2 := go x.f(shared y, shared z) + t3 := go h(shared x, shared y, shared z) + for _ in 0 .. 10000 { + lock z, y, x { + tmp := y.a + y.a = x.a + x.a = z.a + z.a = tmp + } + } + t1.wait() + t2.wait() + t3.wait() + rlock x, y, z{ + assert x.a == 7 + assert y.a == 1 + assert z.a == 5 + } +} diff --git a/vlib/v/tests/shared_lock_6_test.v b/vlib/v/tests/shared_lock_6_test.v new file mode 100644 index 0000000000..f2a9b7e44a --- /dev/null +++ b/vlib/v/tests/shared_lock_6_test.v @@ -0,0 +1,59 @@ +struct St { +mut: + a int +} + +fn (shared x St) f(shared y St, shared z St) { + for _ in 0 .. 10000 { + lock x; rlock y, z { + x.a = y.a + z.a + if x.a > 1000000 { + x.a /= 2 + } + } + } +} + +fn (shared x St) g(shared y St, shared z St) { + for _ in 0 .. 10000 { + rlock x; lock y, z { + y.a += x.a + if y.a > 1000000 { + y.a /= 2 + } + z.a += x.a + if z.a > 1000000 { + z.a /= 2 + } + } + } +} + +fn test_shared_receiver_lock() { + shared x := St{ + a: 5 + } + shared y := St{ + a: 7 + } + shared z := St{ + a: 103 + } + t1 := go x.f(shared y, shared x) + t2 := go y.f(shared y, shared z) + t3 := go z.g(shared x, shared z) + t4 := go x.g(shared x, shared x) + t5 := go z.f(shared z, shared z) + t1.wait() + t2.wait() + t3.wait() + t4.wait() + t5.wait() + // the result is unpredictable - but should be positive + rlock x, y, z { + assert x.a > 10000 + assert y.a > 10000 + assert z.a > 10000 + println('$x.a $y.a $z.a') + } +} diff --git a/vlib/v/tests/shared_unordered_mixed_test.v b/vlib/v/tests/shared_unordered_mixed_test.v new file mode 100644 index 0000000000..3985e707d5 --- /dev/null +++ b/vlib/v/tests/shared_unordered_mixed_test.v @@ -0,0 +1,66 @@ +// integer values from -2^191 .. 2^191-1 +struct Large { +mut: + l u64 + m u64 + h u64 +} + +fn (a Large) clone() Large { + r := Large{ l: a.l, m: a.m h: a.h } + return r +} + +fn (mut a Large) add(b Large) { + oldl := a.l + a.l += b.l + oldm := a.m + if a.l < oldl { a.m++ } + a.m += b.m + if a.m < oldm || (a.l < oldl && a.m <= oldm) { a.h++ } + a.h += b.h +} + +fn doub_large(shared a Large, shared b Large, shared c Large, shared d Large, shared e Large) { + for _ in 0..50 { + lock a, b; rlock c, d, e { + // double the sum by adding all objects to a or b + old_a := a.clone() + a.add(b) + b.add(old_a) + a.add(c) + b.add(d) + a.add(e) + } + } +} + +fn test_mixed_order_lock_rlock() { + // initialze objects so that their sum = 1 + shared a := Large{ l: 4 } + shared b := Large{ l: u64(-7), m: u64(-1) h: u64(-1) } + shared c := Large{ l: 17 } + shared d := Large{ l: u64(-11), m: u64(-1) h: u64(-1) } + shared e := Large{ l: u64(-2), m: u64(-1) h: u64(-1) } + // spawn 3 threads for doubling with different orders of objects + t1 := go doub_large(shared a, shared b, shared c, shared d, shared e) + t2 := go doub_large(shared e, shared b, shared a, shared c, shared d) + t3 := go doub_large(shared b, shared a, shared e, shared d, shared c) + t1.wait() + t2.wait() + t3.wait() + // calculate the sum after 3*50 doublings + mut sum := Large{} + rlock a, b, c, d, e { + sum.add(a) + sum.add(b) + sum.add(c) + sum.add(d) + sum.add(e) + } + // the sum should be 2^150 + assert sum.l == 0 + assert sum.m == 0 + assert sum.h == 4194304 +} +