On Sun, Jan 30, 2022 at 01:33:29AM +0200, Nir Soffer wrote: Wording suggestion: The benchmarks show the efficiency of allocating a new buffer, zeroing it, and interfacing with Go code. Either "These tests" or "This test" As discussed in a different thread, the numbering here groups somewhat-related functionality, and helps us keep cross-language tests correlated over testing the same features. Since you aren't adding counterpart tests to python or ocaml, I don't know what number would be best. But our existing numbering is more along the lines of 0xx for language-level loading, 1xx for NBD handle tests, 2xx for connection tests, 3xx for initial APIs after connecting, 4xx for synchronous APIs, 5xx for asynch APIs, and 6xx for high-level usage patterns. This feels like it might fit better in the 0xx series, since the benchmark does not use any NBD handle. You may want to add 2022. Take the rest of my review with a grain of salt; I'm not (yet?) a Go expert. uninitialized another an uninitialized How long do these benchmark tests take to run? The longer you run a benchmark, the more accurate of a number we can give for a per-operation average, but it also eats up a lot of CPU. Is this something we want running on every 'make check' to ensure no regressions (since it doesn't even involve an NBD handle)? Is there a way to tune things so that 'make check' runs a bare-bones version to avoid bit rot, but another 'make benchmark' (or some such name) runs the full length of a benchmark? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

If a Go program tries to use AioBuffer after calling AioBuffer.Free(), the program may silently corrupt data, accessing memory that does not belong to the buffer any more, or segfault if the address is not mapped. In the worst case, it can corrupt memory silently. Calling Free() twice may silently free unrelated memory. Make the buffer safer to use by Freeing only on the first call and setting the pointer to nil. This makes multiple calls to Free() harmless, just like the underlying C.free(). Trying to access Bytes() and Get() after calling Free() will always panic now, revealing the bug in the program. Trying to use AioBuffer with libnbd API will likely segfault and panic. I did not try to test this. Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; --- golang/aio_buffer.go | 5 +++- golang/libnbd_620_aio_buffer_test.go | 41 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 2bc69a01..2b77d6ee 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -46,20 +46,23 @@ func MakeAioBuffer(size uint) AioBuffer { func FromBytes(buf []byte) AioBuffer { size := len(buf) ret := MakeAioBuffer(uint(size)) for i := 0; i < len(buf); i++ { *ret.Get(uint(i)) = buf[i] } return ret } func (b *AioBuffer) Free() { - C.free(b.P) + if b.P != nil { + C.free(b.P) + b.P = nil + } } func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } diff --git a/golang/libnbd_620_aio_buffer_test.go b/golang/libnbd_620_aio_buffer_test.go index 2632f87f..89badfc6 100644 --- a/golang/libnbd_620_aio_buffer_test.go +++ b/golang/libnbd_620_aio_buffer_test.go @@ -53,20 +53,61 @@ func TestAioBuffer(t *testing.T) { /* Create a nother buffer from Go slice. */ buf2 := FromBytes(zeroes) defer buf2.Free() if !bytes.Equal(buf2.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) } } +func TestAioBufferFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + + /* Free the underlying C array. */ + buf.Free() + + /* And clear the pointer. */ + if buf.P != nil { + t.Fatal("Dangling pointer after Free()") + } + + /* Additional Free does nothing. */ + buf.Free() +} + +func TestAioBufferBytesAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Bytes() after Free()") + } + }() + + buf.Bytes() +} + +func TestAioBufferGetAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Get() after Free()") + } + }() + + *buf.Get(0) = 42 +} + // Typical buffer size. const bufferSize uint = 256 * 1024 // Benchmark creating uninitilized buffer. func BenchmarkMakeAioBuffer(b *testing.B) { for i := 0; i < b.N; i++ { buf := MakeAioBuffer(bufferSize) buf.Free() } } -- 2.34.1

Add standard function documentation comments. The documentation should be available here: https://pkg.go.dev/libguestfs.org/libnbd#AioBuffer Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; --- golang/aio_buffer.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index 2b77d6ee..8f33f500 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -32,37 +32,49 @@ package libnbd import "C" import "unsafe" /* Asynchronous I/O buffer. */ type AioBuffer struct { P unsafe.Pointer Size uint } +// MakeAioBuffer makes a new buffer backed by an unitilialized C allocated +// array. func MakeAioBuffer(size uint) AioBuffer { return AioBuffer{C.malloc(C.ulong(size)), size} } +// FromBytes makes a new buffer backed by a C allocated array, initialized by +// copying the given Go slice. func FromBytes(buf []byte) AioBuffer { size := len(buf) ret := MakeAioBuffer(uint(size)) for i := 0; i < len(buf); i++ { *ret.Get(uint(i)) = buf[i] } return ret } +// Free deallocates the underlying C allocated array. Using the buffer after +// Free() will panic. func (b *AioBuffer) Free() { if b.P != nil { C.free(b.P) b.P = nil } } +// Bytes copies the underlying C array to Go allocated memory and return a +// slice. Modifying the returned slice does not modify the unerlying buffer +// backking array. func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } +// Get returns a pointer to a byte in the underlying C array. The pointer can +// be used to modify the underlying array. The pointer must not be used after +// caling Free(). func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } -- 2.34.1

AioBuffer.Bytes() cannot be used for coping images from NBD to other APis because it copies the entire image. Add a new Slice() function, creating a slice backed by the underling buffer. Using Slice() is efficient, but less safe, like Get(). The returned slice must be used only before calling Free(). This should not be an issue with typical code. Testing show that Slice() is much faster than Bytes() for typical 256k buffer: BenchmarkAioBufferBytes-12 86616 16529 ns/op BenchmarkAioBufferSlice-12 1000000000 0.4630 ns/op I modified the aio_copy example to use AioBuffer and complied 2 versions, one using Bytes() and one using Slice(). When copying 6g fully allocated image, the version using Slice() is 1.6 times faster. $ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC >/dev/null" Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 3.357 s ± 0.039 s [User: 2.656 s, System: 1.162 s] Range (min … max): 3.313 s … 3.387 s 3 runs Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 2.046 s ± 0.009 s [User: 0.423 s, System: 0.892 s] Range (min … max): 2.037 s … 2.055 s 3 runs Summary './aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran 1.64 ± 0.02 times faster than './aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null' When copying a 6g empty image (qemu-nbd sends one hole chunk for every read), the version using Slice() is 2.6 times faster. $ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC >/dev/null" Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 1.210 s ± 0.023 s [User: 1.428 s, System: 0.345 s] Range (min … max): 1.191 s … 1.235 s 3 runs Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null Time (mean ± σ): 461.4 ms ± 13.1 ms [User: 394.2 ms, System: 76.6 ms] Range (min … max): 450.6 ms … 476.0 ms 3 runs Summary './aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran 2.62 ± 0.09 times faster than './aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null' Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; --- golang/aio_buffer.go | 9 ++++++ golang/libnbd_620_aio_buffer_test.go | 41 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go index dcb036a5..788fe1a8 100644 --- a/golang/aio_buffer.go +++ b/golang/aio_buffer.go @@ -71,16 +71,25 @@ func (b *AioBuffer) Free() { } } // Bytes copies the underlying C array to Go allocated memory and return a // slice. Modifying the returned slice does not modify the unerlying buffer // backking array. func (b *AioBuffer) Bytes() []byte { return C.GoBytes(b.P, C.int(b.Size)) } +// Slice creates a slice backed by the underlying C array. The slice can be +// used to access or modify the contents of the underlying array. The slice +// must not be used after caling Free(). +func (b *AioBuffer) Slice() []byte { + // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices + // TODO: Use unsafe.Slice() when we require Go 1.17. + return (*[1<<30]byte)(b.P)[:b.Size:b.Size] +} + // Get returns a pointer to a byte in the underlying C array. The pointer can // be used to modify the underlying array. The pointer must not be used after // caling Free(). func (b *AioBuffer) Get(i uint) *byte { return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i))) } diff --git a/golang/libnbd_620_aio_buffer_test.go b/golang/libnbd_620_aio_buffer_test.go index 53d6233b..38c6c74d 100644 --- a/golang/libnbd_620_aio_buffer_test.go +++ b/golang/libnbd_620_aio_buffer_test.go @@ -44,20 +44,35 @@ func TestAioBuffer(t *testing.T) { /* Modifying returned slice does not modify the buffer. */ for i := 0; i < len(b); i++ { b[i] = 42 } /* Bytes() still returns zeroes. */ if !bytes.Equal(buf.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes()) } + /* Creating a slice without copying the underlhing buffer. */ + s := buf.Slice() + if !bytes.Equal(s, zeroes) { + t.Fatalf("Expected %v, got %v", zeroes, s) + } + + /* Modifing the slice modifies the underlying buffer. */ + for i := 0; i < len(s); i++ { + s[i] = 42 + } + + if !bytes.Equal(buf.Slice(), s) { + t.Fatalf("Expected %v, got %v", s, buf.Slice()) + } + /* Create a nother buffer from Go slice. */ buf2 := FromBytes(zeroes) defer buf2.Free() if !bytes.Equal(buf2.Bytes(), zeroes) { t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes()) } /* Crated a zeroed buffer. */ buf3 := MakeAioBufferZero(uint(32)) @@ -89,20 +104,34 @@ func TestAioBufferBytesAfterFree(t *testing.T) { defer func() { if r := recover(); r == nil { t.Fatal("Did not recover from panic calling Bytes() after Free()") } }() buf.Bytes() } +func TestAioBufferSliceAfterFree(t *testing.T) { + buf := MakeAioBuffer(uint(32)) + buf.Free() + + defer func() { + if r := recover(); r == nil { + t.Fatal("Did not recover from panic calling Bytes() after Free()") + } + }() + + s := buf.Slice() + s[0] = 42 +} + func TestAioBufferGetAfterFree(t *testing.T) { buf := MakeAioBuffer(uint(32)) buf.Free() defer func() { if r := recover(); r == nil { t.Fatal("Did not recover from panic calling Get() after Free()") } }() @@ -152,10 +181,22 @@ func BenchmarkFromBytes(b *testing.B) { func BenchmarkAioBufferBytes(b *testing.B) { buf := MakeAioBuffer(bufferSize) defer buf.Free() var r int b.ResetTimer() for i := 0; i < b.N; i++ { r += len(buf.Bytes()) } } + +// Benchmark creating a slice without copying the underling buffer. +func BenchmarkAioBufferSlice(b *testing.B) { + buf := MakeAioBuffer(bufferSize) + defer buf.Free() + var r int + + b.ResetTimer() + for i := 0; i < b.N; i++ { + r += len(buf.Slice()) + } +} -- 2.34.1

On Tue, Feb 1, 2022 at 3:12 PM Eric Blake <eblake(a)redhat.com&gt; wrote: Will fix in v2

On Sun, Jan 30, 2022 at 01:33:36AM +0200, Nir Soffer wrote: copying will be much slower, hiding the overhead of allocating or zeroing buffers. Looks like you had a blank line with trailing whitespace in an earlier patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Tue, Feb 1, 2022 at 3:15 PM Eric Blake <eblake(a)redhat.com&gt; wrote: I could not find a blank line with trailing whitespace in this series. Did you finish reviewing this change? Richard already acked this, but since this adds a new API, I would like to get more feedback. Nir