thr3ads.net - Libguestfs - [Libguestfs] [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use [Feb 2022]

If this information is useful, please help other people find it:
Share via:

Nir Soffer

2022-Jan-29 23:33 UTC

[Libguestfs] [PATCH libnbd 0/9] golang: Safer, easier to use, and faster AioBuffer

Improve AioBuffer to make it safer, easier to use, and faster when integrating
with other Go APIs.

New Go specific APIs:

- MakeAioBufferZero() - creates a new buffer using calloc(), to make it easy
  and efficient to use a zeroed buffer.

- AioBuffer.Slice() - create a slice backed by the underlying buffer without
  copying the contents of the buffer.

Performance improments:

- FromBytes() is 3 time faster
- Code using Bytes() should use Slice() now. aio_copy example shows up to 260%
  speedup.

Improve testing:

- New AioBuffer tests
- New AioBuffer benchmarks

Documention:

- AioBuffer is fully documnted now.

Nir Soffer (9):
  golang: tests: Add test for AioBuffer
  golang: aio_buffer.go: Make it safer to use
  golang: aio_buffer.go: Add missing documentation
  golang: aio_buffer.go: Add MakeAioBufferZero()
  golang: aio_buffer.go: Add Slice()
  golang: tests: Use AioBuffer.Slice()
  golang: aio_buffer.go: Speed up FromBytes()
  golang: aio_buffer.go: Benchmark copy flows
  golang: examples: aio_copy: Simplify using AioBuffer

 golang/Makefile.am                   |   1 +
 golang/aio_buffer.go                 |  39 ++++-
 golang/examples/aio_copy/aio_copy.go |  29 +---
 golang/libnbd_500_aio_pread_test.go  |   2 +-
 golang/libnbd_510_aio_pwrite_test.go |   8 +-
 golang/libnbd_620_aio_buffer_test.go | 236 +++++++++++++++++++++++++++
 6 files changed, 281 insertions(+), 34 deletions(-)
 create mode 100644 golang/libnbd_620_aio_buffer_test.go

-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 1/9] golang: tests: Add test for AioBuffer

Add unit tests and benchmarks for AioBuffer. The tests are trivial but
they server as running documentation, and they point out important
details about the type.

The benchmarks how efficient is allocation a new buffer, zeroing it, and
interfacing with Go code.

This tests will also ensure that we don't break anything by the next
changes.

To run the benchmarks use:

$ go test -run=xxx -bench=.
goos: linux
goarch: amd64
pkg: libguestfs.org/libnbd
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkMakeAioBuffer-12     	 6871759	       157.2 ns/op
BenchmarkAioBufferZero-12     	   17551	     69552 ns/op
BenchmarkFromBytes-12         	    9632	    139112 ns/op
BenchmarkAioBufferBytes-12    	   69375	     16410 ns/op
PASS
ok  	libguestfs.org/libnbd	5.843s

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/Makefile.am                   |   1 +
 golang/libnbd_620_aio_buffer_test.go | 104 +++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 golang/libnbd_620_aio_buffer_test.go

diff --git a/golang/Makefile.am b/golang/Makefile.am
index 10fb8934..ae0486dd 100644
--- a/golang/Makefile.am
+++ b/golang/Makefile.am
@@ -37,20 +37,21 @@ source_files = \
 	libnbd_300_get_size_test.go \
 	libnbd_400_pread_test.go \
 	libnbd_405_pread_structured_test.go \
 	libnbd_410_pwrite_test.go \
 	libnbd_460_block_status_test.go \
 	libnbd_500_aio_pread_test.go \
 	libnbd_510_aio_pwrite_test.go \
 	libnbd_590_aio_copy_test.go \
 	libnbd_600_debug_callback_test.go \
 	libnbd_610_error_test.go \
+	libnbd_620_aio_buffer_test.go \
 	$(NULL)
 
 generator_built = \
 	bindings.go \
 	closures.go \
 	wrappers.go \
 	wrappers.h \
 	$(NULL)
 
 EXTRA_DIST = \
diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
new file mode 100644
index 00000000..2632f87f
--- /dev/null
+++ b/golang/libnbd_620_aio_buffer_test.go
@@ -0,0 +1,104 @@
+/* libnbd golang tests
+ * Copyright (C) 2013-2021 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+package libnbd
+
+import (
+	"bytes"
+	"testing"
+)
+
+func TestAioBuffer(t *testing.T) {
+	/* Create a buffer with unitinialized backing array. */
+	buf := MakeAioBuffer(uint(32))
+	defer buf.Free()
+
+	/* Initialize backing array contents. */
+	for i := uint(0); i < buf.Size; i++ {
+		*buf.Get(i) = 0
+	}
+
+	/* Create a slice by copying the backing arrary contents into Go memory. */
+	b := buf.Bytes()
+
+	zeroes := make([]byte, 32)
+	if !bytes.Equal(b, zeroes) {
+		t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
+	}
+
+	/* 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())
+	}
+
+	/* 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())
+	}
+}
+
+// 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()
+	}
+}
+
+// Benchmark zeroing a buffer.
+func BenchmarkAioBufferZero(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		buf := MakeAioBuffer(bufferSize)
+		for i := uint(0); i < bufferSize; i++ {
+			*buf.Get(i) = 0
+		}
+		buf.Free()
+	}
+}
+
+// Benchmark creating a buffer by coping a Go slice.
+func BenchmarkFromBytes(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		zeroes := make([]byte, bufferSize)
+		buf := FromBytes(zeroes)
+		buf.Free()
+	}
+}
+
+// Benchmark creating a slice by copying the contents.
+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())
+	}
+}
-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use

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 at redhat.com>
---
 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

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 3/9] golang: aio_buffer.go: Add missing documentation

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 at redhat.com>
---
 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

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()

Make it easy to create a zeroed buffer via calloc(), preventing leaking
sensitive info from the heap.

Benchmarking show that creating a zeroed buffer is much slower compared
with uninitialized buffer, but much faster compared with manually
initializing the buffer with a loop.

BenchmarkMakeAioBuffer-12        	 7252674	       148.1 ns/op
BenchmarkMakeAioBufferZero-12    	  262107	      4181 ns/op
BenchmarkAioBufferZero-12        	   17581	     68759 ns/op

It is interesting that creating a zeroed buffer is 3 times faster
compared with making a new []byte slice:

BenchmarkMakeAioBufferZero-12    	  247710	      4440 ns/op
BenchmarkMakeByteSlice-12        	   84117	     13733 ns/op

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/aio_buffer.go                 |  6 ++++++
 golang/libnbd_620_aio_buffer_test.go | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 8f33f500..dcb036a5 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -38,20 +38,26 @@ 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}
 }
 
+// MakeAioBuffer makes a new buffer backed by a C allocated array. The
+// underlying buffer is set to zero.
+func MakeAioBufferZero(size uint) AioBuffer {
+	return AioBuffer{C.calloc(C.ulong(1), 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
 }
diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
index 89badfc6..53d6233b 100644
--- a/golang/libnbd_620_aio_buffer_test.go
+++ b/golang/libnbd_620_aio_buffer_test.go
@@ -51,20 +51,28 @@ func TestAioBuffer(t *testing.T) {
 		t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
 	}
 
 	/* 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))
+	defer buf.Free()
+
+	if !bytes.Equal(buf3.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 {
@@ -105,20 +113,28 @@ func TestAioBufferGetAfterFree(t *testing.T) {
 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()
 	}
 }
 
+// Benchmark creating zeroed buffer.
+func BenchmarkMakeAioBufferZero(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		buf := MakeAioBufferZero(bufferSize)
+		buf.Free()
+	}
+}
+
 // Benchmark zeroing a buffer.
 func BenchmarkAioBufferZero(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		buf := MakeAioBuffer(bufferSize)
 		for i := uint(0); i < bufferSize; i++ {
 			*buf.Get(i) = 0
 		}
 		buf.Free()
 	}
 }
-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 5/9] golang: aio_buffer.go: Add Slice()

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 at redhat.com>
---
 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

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 6/9] golang: tests: Use AioBuffer.Slice()

Slice() is easier to use and faster than Get() or Bytes(). Lets use the
new way.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/libnbd_500_aio_pread_test.go  | 2 +-
 golang/libnbd_510_aio_pwrite_test.go | 8 +++++---
 golang/libnbd_620_aio_buffer_test.go | 8 +++++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/golang/libnbd_500_aio_pread_test.go
b/golang/libnbd_500_aio_pread_test.go
index 0811378c..bd0208ef 100644
--- a/golang/libnbd_500_aio_pread_test.go
+++ b/golang/libnbd_500_aio_pread_test.go
@@ -55,14 +55,14 @@ func Test500AioPRead(t *testing.T) {
 		}
 		h.Poll(-1)
 	}
 
 	// Expected data.
 	expected := make([]byte, 512)
 	for i := 0; i < 512; i += 8 {
 		binary.BigEndian.PutUint64(expected[i:i+8], uint64(i))
 	}
 
-	if !bytes.Equal(buf.Bytes(), expected) {
+	if !bytes.Equal(buf.Slice(), expected) {
 		t.Fatalf("did not read expected data")
 	}
 }
diff --git a/golang/libnbd_510_aio_pwrite_test.go
b/golang/libnbd_510_aio_pwrite_test.go
index 56cdcb05..493159f2 100644
--- a/golang/libnbd_510_aio_pwrite_test.go
+++ b/golang/libnbd_510_aio_pwrite_test.go
@@ -32,23 +32,25 @@ func Test510AioPWrite(t *testing.T) {
 		"nbdkit", "-s", "--exit-with-parent",
"-v",
 		"memory", "size=512",
 	})
 	if err != nil {
 		t.Fatalf("could not connect: %s", err)
 	}
 
 	/* Write a pattern and read it back. */
 	buf := MakeAioBuffer(512)
 	defer buf.Free()
+
+	s := buf.Slice()
 	for i := 0; i < 512; i += 2 {
-		*buf.Get(uint(i)) = 0x55
-		*buf.Get(uint(i + 1)) = 0xAA
+		s[i] = 0x55
+		s[i+1] = 0xAA
 	}
 
 	var cookie uint64
 	cookie, err = h.AioPwrite(buf, 0, nil)
 	if err != nil {
 		t.Fatalf("%s", err)
 	}
 	for {
 		var b bool
 		b, err = h.AioCommandCompleted(cookie)
@@ -62,14 +64,14 @@ func Test510AioPWrite(t *testing.T) {
 	}
 
 	/* We already tested aio_pread, let's just read the data
 	back in the regular synchronous way. */
 	buf2 := make([]byte, 512)
 	err = h.Pread(buf2, 0, nil)
 	if err != nil {
 		t.Fatalf("%s", err)
 	}
 
-	if !bytes.Equal(buf.Bytes(), buf2) {
+	if !bytes.Equal(buf.Slice(), buf2) {
 		t.Fatalf("did not read back same data as written")
 	}
 }
diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
index 38c6c74d..8f65b558 100644
--- a/golang/libnbd_620_aio_buffer_test.go
+++ b/golang/libnbd_620_aio_buffer_test.go
@@ -22,22 +22,23 @@ import (
 	"bytes"
 	"testing"
 )
 
 func TestAioBuffer(t *testing.T) {
 	/* Create a buffer with unitinialized backing array. */
 	buf := MakeAioBuffer(uint(32))
 	defer buf.Free()
 
 	/* Initialize backing array contents. */
+	s := buf.Slice()
 	for i := uint(0); i < buf.Size; i++ {
-		*buf.Get(i) = 0
+		s[i] = 0
 	}
 
 	/* Create a slice by copying the backing arrary contents into Go memory. */
 	b := buf.Bytes()
 
 	zeroes := make([]byte, 32)
 	if !bytes.Equal(b, zeroes) {
 		t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
 	}
 
@@ -45,21 +46,21 @@ func TestAioBuffer(t *testing.T) {
 	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()
+	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) {
@@ -154,22 +155,23 @@ func BenchmarkMakeAioBufferZero(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		buf := MakeAioBufferZero(bufferSize)
 		buf.Free()
 	}
 }
 
 // Benchmark zeroing a buffer.
 func BenchmarkAioBufferZero(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		buf := MakeAioBuffer(bufferSize)
+		s := buf.Slice()
 		for i := uint(0); i < bufferSize; i++ {
-			*buf.Get(i) = 0
+			s[i] = 0
 		}
 		buf.Free()
 	}
 }
 
 // Benchmark creating a buffer by coping a Go slice.
 func BenchmarkFromBytes(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		zeroes := make([]byte, bufferSize)
 		buf := FromBytes(zeroes)
-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 7/9] golang: aio_buffer.go: Speed up FromBytes()

Using Slice() we can use builtin copy() instead of a manual loop, which
is 4.6 times faster with a typical 256k buffer:

Before:
BenchmarkFromBytes-12            	    9806	    111474 ns/op

After:
BenchmarkFromBytes-12            	   48193	     24106 ns/op

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/aio_buffer.go | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 788fe1a8..7960e554 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -47,25 +47,22 @@ func MakeAioBuffer(size uint) AioBuffer {
 
 // MakeAioBuffer makes a new buffer backed by a C allocated array. The
 // underlying buffer is set to zero.
 func MakeAioBufferZero(size uint) AioBuffer {
 	return AioBuffer{C.calloc(C.ulong(1), 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]
-	}
+	ret := MakeAioBuffer(uint(len(buf)))
+	copy(ret.Slice(), buf)
 	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
 	}
-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 8/9] golang: aio_buffer.go: Benchmark copy flows

Add benchmark for coping a buffer using 3 strategies - reusing same
buffer, making a new uninitialized buffer per copy, and using a zeroed
buffer per copy. This benchmark is the worst possible case, coping a
buffer to memory. Any real I/O will be much slower hiding the overhead
of allocating buffer or zeroed buffered.

$ go test -run=AioBuffer -bench=Copy -benchtime=5s
goos: linux
goarch: amd64
pkg: libguestfs.org/libnbd
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkAioBufferCopyBaseline-12    	 1142508	      4523 ns/op
BenchmarkAioBufferCopyMake-12        	 1000000	      5320 ns/op
BenchmarkAioBufferCopyMakeZero-12    	  728940	      8218 ns/op

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/libnbd_620_aio_buffer_test.go | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
index 8f65b558..9ab411b3 100644
--- a/golang/libnbd_620_aio_buffer_test.go
+++ b/golang/libnbd_620_aio_buffer_test.go
@@ -195,10 +195,42 @@ func BenchmarkAioBufferBytes(b *testing.B) {
 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())
 	}
 }
+
+var data = make([]byte, bufferSize)
+
+// Benchmark copying into same buffer, used as baseline for CopyMake and
+// CopyMakeZero benchmarks.
+func BenchmarkAioBufferCopyBaseline(b *testing.B) {
+	buf := MakeAioBufferZero(bufferSize)
+	defer buf.Free()
+
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		copy(buf.Slice(), data)
+	}
+}
+
+// Benchmark overhead of making a new buffer per read.
+func BenchmarkAioBufferCopyMake(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		buf := MakeAioBuffer(bufferSize)
+		copy(buf.Slice(), data)
+		buf.Free()
+	}
+}
+
+// Benchmark overhead of making a new zero buffer per read.
+func BenchmarkAioBufferCopyMakeZero(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		buf := MakeAioBufferZero(bufferSize)
+		copy(buf.Slice(), data)
+		buf.Free()
+	}
+}
-- 
2.34.1

Nir Soffer

2022-Jan-29 23:33 UTC

head link

[Libguestfs] [PATCH libnbd 9/9] golang: examples: aio_copy: Simplify using AioBuffer

Now that we have an efficient way to use AioBuffer, we don't need the
hacks to create AioBuffer from Go slice.

Benchmarking AioBuffer show that allocating a 256k buffer is practically
free, so there is no need for the buffer pool. Now we allocate a new
buffer per request, keep it in the command, and free it when the request
is finished.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/examples/aio_copy/aio_copy.go | 29 +++++-----------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/golang/examples/aio_copy/aio_copy.go
b/golang/examples/aio_copy/aio_copy.go
index b6f5def1..bb20b478 100644
--- a/golang/examples/aio_copy/aio_copy.go
+++ b/golang/examples/aio_copy/aio_copy.go
@@ -37,53 +37,43 @@
 // Example:
 //
 //   ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
 //
 package main
 
 import (
 	"container/list"
 	"flag"
 	"os"
-	"sync"
 	"syscall"
-	"unsafe"
 
 	"libguestfs.org/libnbd"
 )
 
 var (
 	// These options give best performance with fast NVMe drive.
 	requestSize = flag.Uint("request-size", 256*1024, "maximum
request size in bytes")
 	requests    = flag.Uint("requests", 4, "maximum number of
requests in flight")
 
 	h *libnbd.Libnbd
 
 	// Keeping commands in a queue ensures commands are written in the right
 	// order, even if they complete out of order. This allows parallel reads
 	// with non-seekable output.
 	queue list.List
-
-	// Buffer pool allocating buffers as needed and reusing them.
-	bufPool = sync.Pool{
-		New: func() interface{} {
-			return make([]byte, *requestSize)
-		},
-	}
 )
 
 // command keeps state of single AioPread call while the read is handled by
 // libnbd, until the command reach the front of the queue and can be writen to
 // the output.
 type command struct {
-	buf    []byte
-	length uint
+	buf    libnbd.AioBuffer
 	ready  bool
 }
 
 func main() {
 	flag.Parse()
 
 	var err error
 
 	h, err = libnbd.Create()
 	if err != nil {
@@ -139,60 +129,51 @@ func waitForCompletion() {
 			panic(err)
 		}
 
 		if inflightRequests() < start {
 			break // A read completed.
 		}
 	}
 }
 
 func startRead(offset uint64, length uint) {
-	buf := bufPool.Get().([]byte)
-
-	// Keep buffer in command so we can put it back into the pool when the
-	// command completes.
-	cmd := &command{buf: buf, length: length}
-
-	// Create aio buffer from pool buffer to avoid unneeded allocation for
-	// every read, and unneeded copy when completing the read.
-	abuf := libnbd.AioBuffer{P: unsafe.Pointer(&buf[0]), Size: length}
+	cmd := &command{buf: libnbd.MakeAioBuffer(length)}
 
 	args := libnbd.AioPreadOptargs{
 		CompletionCallbackSet: true,
 		CompletionCallback: func(error *int) int {
 			if *error != 0 {
 				// This is not documented, but *error is errno value translated
 				// from the the NBD server error.
 				err := syscall.Errno(*error).Error()
 				panic(err)
 			}
 			cmd.ready = true
 			return 1
 		},
 	}
 
-	_, err := h.AioPread(abuf, offset, &args)
+	_, err := h.AioPread(cmd.buf, offset, &args)
 	if err != nil {
 		panic(err)
 	}
 
 	queue.PushBack(cmd)
 }
 
 func readReady() bool {
 	return queue.Len() > 0 && queue.Front().Value.(*command).ready
 }
 
 func finishRead() {
 	e := queue.Front()
 	queue.Remove(e)
 
 	cmd := e.Value.(*command)
-	b := cmd.buf[:cmd.length]
 
-	_, err := os.Stdout.Write(b)
+	_, err := os.Stdout.Write(cmd.buf.Slice())
 	if err != nil {
 		panic(err)
 	}
 
-	bufPool.Put(cmd.buf)
+	cmd.buf.Free()
 }
-- 
2.34.1

Eric Blake

2022-Feb-01 13:09 UTC

head link

[Libguestfs] [PATCH libnbd 3/9] golang: aio_buffer.go: Add missing documentation

On Sun, Jan 30, 2022 at 01:33:31AM +0200, Nir Soffer
wrote:> 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 at redhat.com>
> ---
>  
> +// MakeAioBuffer makes a new buffer backed by an unitilialized C allocated
uninitialized
> +// 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
underlying
> +// backking array.
backing
>  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
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake

2022-Feb-01 13:11 UTC

head link

[Libguestfs] [PATCH libnbd 5/9] golang: aio_buffer.go: Add Slice()

On Sun, Jan 30, 2022 at 01:33:33AM +0200, Nir Soffer
wrote:> AioBuffer.Bytes() cannot be used for coping images from NBD to other
copying
> APis because it copies the entire image. Add a new Slice() function,
> creating a slice backed by the underling buffer.
underlying
> 
> 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
compiled
> +++ 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. */
underlying
> +	s := buf.Slice()
> +	if !bytes.Equal(s, zeroes) {
> +		t.Fatalf("Expected %v, got %v", zeroes, s)
> +	}
> +
> +	/* Modifing the slice modifies the underlying buffer. */
Modifying
>  }
> +
> +// Benchmark creating a slice without copying the underling buffer.
underlying
> +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
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake

2022-Feb-01 13:14 UTC

head link

[Libguestfs] [PATCH libnbd 8/9] golang: aio_buffer.go: Benchmark copy flows

On Sun, Jan 30, 2022 at 01:33:36AM +0200, Nir Soffer
wrote:> Add benchmark for coping a buffer using 3 strategies - reusing same
> buffer, making a new uninitialized buffer per copy, and using a zeroed
> buffer per copy. This benchmark is the worst possible case, coping a
copying
> buffer to memory. Any real I/O will be much slower hiding the overhead
> of allocating buffer or zeroed buffered.
will be much slower, hiding the overhead of allocating or zeroing buffers.
> 
> $ go test -run=AioBuffer -bench=Copy -benchtime=5s
> goos: linux
> goarch: amd64
> pkg: libguestfs.org/libnbd
> cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
> BenchmarkAioBufferCopyBaseline-12    	 1142508	      4523 ns/op
> BenchmarkAioBufferCopyMake-12        	 1000000	      5320 ns/op
> BenchmarkAioBufferCopyMakeZero-12    	  728940	      8218 ns/op
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  golang/libnbd_620_aio_buffer_test.go | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
> index 8f65b558..9ab411b3 100644
> --- a/golang/libnbd_620_aio_buffer_test.go
> +++ b/golang/libnbd_620_aio_buffer_test.go
> @@ -195,10 +195,42 @@ func BenchmarkAioBufferBytes(b *testing.B) {
>  func BenchmarkAioBufferSlice(b *testing.B) {
>  	buf := MakeAioBuffer(bufferSize)
>  	defer buf.Free()
>  	var r int
>  
>  	b.ResetTimer()
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

Eric Blake

2022-Feb-08 19:33 UTC

head link

[Libguestfs] [PATCH libnbd 1/9] golang: tests: Add test for AioBuffer

On Sun, Jan 30, 2022 at 01:33:29AM +0200, Nir Soffer
wrote:> Add unit tests and benchmarks for AioBuffer. The tests are trivial but
> they server as running documentation, and they point out important
> details about the type.
> 
> The benchmarks how efficient is allocation a new buffer, zeroing it, and
> interfacing with Go code.
Wording suggestion:

The benchmarks show the efficiency of allocating a new buffer, zeroing
it, and interfacing with Go code.
> 
> This tests will also ensure that we don't break anything by the next
Either "These tests" or "This test"
> changes.
> 
> To run the benchmarks use:
> 
> $ go test -run=xxx -bench=.
> goos: linux
> goarch: amd64
> pkg: libguestfs.org/libnbd
> cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
> BenchmarkMakeAioBuffer-12     	 6871759	       157.2 ns/op
> BenchmarkAioBufferZero-12     	   17551	     69552 ns/op
> BenchmarkFromBytes-12         	    9632	    139112 ns/op
> BenchmarkAioBufferBytes-12    	   69375	     16410 ns/op
> PASS
> ok  	libguestfs.org/libnbd	5.843s
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  golang/Makefile.am                   |   1 +
>  golang/libnbd_620_aio_buffer_test.go | 104 +++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
>  create mode 100644 golang/libnbd_620_aio_buffer_test.go
> 
> diff --git a/golang/Makefile.am b/golang/Makefile.am
> index 10fb8934..ae0486dd 100644
> --- a/golang/Makefile.am
> +++ b/golang/Makefile.am
> @@ -37,20 +37,21 @@ source_files = \
>  	libnbd_300_get_size_test.go \
>  	libnbd_400_pread_test.go \
>  	libnbd_405_pread_structured_test.go \
>  	libnbd_410_pwrite_test.go \
>  	libnbd_460_block_status_test.go \
>  	libnbd_500_aio_pread_test.go \
>  	libnbd_510_aio_pwrite_test.go \
>  	libnbd_590_aio_copy_test.go \
>  	libnbd_600_debug_callback_test.go \
>  	libnbd_610_error_test.go \
> +	libnbd_620_aio_buffer_test.go \
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.
>  	$(NULL)
>  
>  generator_built = \
>  	bindings.go \
>  	closures.go \
>  	wrappers.go \
>  	wrappers.h \
>  	$(NULL)
>  
>  EXTRA_DIST = \
> diff --git a/golang/libnbd_620_aio_buffer_test.go
b/golang/libnbd_620_aio_buffer_test.go
> new file mode 100644
> index 00000000..2632f87f
> --- /dev/null
> +++ b/golang/libnbd_620_aio_buffer_test.go
> @@ -0,0 +1,104 @@
> +/* libnbd golang tests
> + * Copyright (C) 2013-2021 Red Hat Inc.
You may want to add 2022.

Take the rest of my review with a grain of salt; I'm not (yet?) a Go expert.
> +
> +package libnbd
> +
> +import (
> +	"bytes"
> +	"testing"
> +)
> +
> +func TestAioBuffer(t *testing.T) {
> +	/* Create a buffer with unitinialized backing array. */
uninitialized
> +	buf := MakeAioBuffer(uint(32))
> +	defer buf.Free()
> +
> +	/* Initialize backing array contents. */
> +	for i := uint(0); i < buf.Size; i++ {
> +		*buf.Get(i) = 0
> +	}
> +
> +	/* Create a slice by copying the backing arrary contents into Go memory.
*/
> +	b := buf.Bytes()
> +
> +	zeroes := make([]byte, 32)
> +	if !bytes.Equal(b, zeroes) {
> +		t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
> +	}
> +
> +	/* 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())
> +	}
> +
> +	/* Create a nother buffer from Go slice. */
another
> +	buf2 := FromBytes(zeroes)
> +	defer buf2.Free()
> +
> +	if !bytes.Equal(buf2.Bytes(), zeroes) {
> +		t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
> +	}
> +}
> +
> +// Typical buffer size.
> +const bufferSize uint = 256 * 1024
> +
> +// Benchmark creating uninitilized buffer.
an uninitialized
> +func BenchmarkMakeAioBuffer(b *testing.B) {
> +	for i := 0; i < b.N; i++ {
> +		buf := MakeAioBuffer(bufferSize)
> +		buf.Free()
> +	}
> +}
> +
> +// Benchmark zeroing a buffer.
> +func BenchmarkAioBufferZero(b *testing.B) {
> +	for i := 0; i < b.N; i++ {
> +		buf := MakeAioBuffer(bufferSize)
> +		for i := uint(0); i < bufferSize; i++ {
> +			*buf.Get(i) = 0
> +		}
> +		buf.Free()
> +	}
> +}
> +
> +// Benchmark creating a buffer by coping a Go slice.
> +func BenchmarkFromBytes(b *testing.B) {
> +	for i := 0; i < b.N; i++ {
> +		zeroes := make([]byte, bufferSize)
> +		buf := FromBytes(zeroes)
> +		buf.Free()
> +	}
> +}
> +
> +// Benchmark creating a slice by copying the contents.
> +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())
> +	}
> +}
> -- 
> 2.34.1
>
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

Eric Blake

2022-Feb-08 20:37 UTC

head link

[Libguestfs] [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use

On Sun, Jan 30, 2022 at 01:33:30AM +0200, Nir Soffer
wrote:> 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 at redhat.com>
> ---
>  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
> +	}
Good.
> +++ b/golang/libnbd_620_aio_buffer_test.go
> @@ -53,20 +53,61 @@ func TestAioBuffer(t *testing.T) {
See patch 1 comments about the file name.  Otherwise looks good.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake

2022-Feb-08 20:44 UTC

head link

[Libguestfs] [PATCH libnbd 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()

On Sun, Jan 30, 2022 at 01:33:32AM +0200, Nir Soffer
wrote:> Make it easy to create a zeroed buffer via calloc(), preventing leaking
> sensitive info from the heap.
> 
> Benchmarking show that creating a zeroed buffer is much slower compared
shows
> with uninitialized buffer, but much faster compared with manually
> initializing the buffer with a loop.
> 
> BenchmarkMakeAioBuffer-12        	 7252674	       148.1 ns/op
> BenchmarkMakeAioBufferZero-12    	  262107	      4181 ns/op
> BenchmarkAioBufferZero-12        	   17581	     68759 ns/op
> 
> It is interesting that creating a zeroed buffer is 3 times faster
> compared with making a new []byte slice:
> 
> BenchmarkMakeAioBufferZero-12    	  247710	      4440 ns/op
> BenchmarkMakeByteSlice-12        	   84117	     13733 ns/op
Some of this is due to how much vectorization the standard library
(whether libc or Go's core libraries) can do when bulk-zeroing
(zeroing 64 bits, or even a cache line at a time, in an unrolled loop,
is always going to be more performant than a naive loop of one byte at
a time).
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  golang/aio_buffer.go                 |  6 ++++++
>  golang/libnbd_620_aio_buffer_test.go | 16 ++++++++++++++++
Another file that may fit better in the 0xx naming, especially if we
decide to duplicate similar functionality into the python or OCaml
bindings of being able to pre-generate a known-zero buffer for use in
passing to nbd_pread.

As a helper API, this seems useful.  But do we need any man page
documentation of a language-specific helper function?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake

2022-Feb-08 20:49 UTC

head link

[Libguestfs] [PATCH libnbd 6/9] golang: tests: Use AioBuffer.Slice()

On Sun, Jan 30, 2022 at 01:33:34AM +0200, Nir Soffer
wrote:> Slice() is easier to use and faster than Get() or Bytes(). Lets use the
Let's
> new way.
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  golang/libnbd_500_aio_pread_test.go  | 2 +-
>  golang/libnbd_510_aio_pwrite_test.go | 8 +++++---
>  golang/libnbd_620_aio_buffer_test.go | 8 +++++---
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/golang/libnbd_500_aio_pread_test.go
b/golang/libnbd_500_aio_pread_test.go
> index 0811378c..bd0208ef 100644
> --- a/golang/libnbd_500_aio_pread_test.go
ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake

2022-Feb-08 20:50 UTC

head link

[Libguestfs] [PATCH libnbd 7/9] golang: aio_buffer.go: Speed up FromBytes()

On Sun, Jan 30, 2022 at 01:33:35AM +0200, Nir Soffer
wrote:> Using Slice() we can use builtin copy() instead of a manual loop, which
> is 4.6 times faster with a typical 256k buffer:
Again, the benefits of the underlying libraries being able to
vectorize over larger chunks of memory ;)
> 
> Before:
> BenchmarkFromBytes-12            	    9806	    111474 ns/op
> 
> After:
> BenchmarkFromBytes-12            	   48193	     24106 ns/op
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  golang/aio_buffer.go | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Libguestfs - Feb 2022 - [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use

[Libguestfs] [PATCH libnbd 0/9] golang: Safer, easier to use, and faster AioBuffer

[Libguestfs] [PATCH libnbd 1/9] golang: tests: Add test for AioBuffer

[Libguestfs] [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use

[Libguestfs] [PATCH libnbd 3/9] golang: aio_buffer.go: Add missing documentation

[Libguestfs] [PATCH libnbd 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()

[Libguestfs] [PATCH libnbd 5/9] golang: aio_buffer.go: Add Slice()

[Libguestfs] [PATCH libnbd 6/9] golang: tests: Use AioBuffer.Slice()

[Libguestfs] [PATCH libnbd 7/9] golang: aio_buffer.go: Speed up FromBytes()

[Libguestfs] [PATCH libnbd 8/9] golang: aio_buffer.go: Benchmark copy flows

[Libguestfs] [PATCH libnbd 9/9] golang: examples: aio_copy: Simplify using AioBuffer

[Libguestfs] [PATCH libnbd 3/9] golang: aio_buffer.go: Add missing documentation

[Libguestfs] [PATCH libnbd 5/9] golang: aio_buffer.go: Add Slice()

[Libguestfs] [PATCH libnbd 8/9] golang: aio_buffer.go: Benchmark copy flows

[Libguestfs] [PATCH libnbd 1/9] golang: tests: Add test for AioBuffer

[Libguestfs] [PATCH libnbd 2/9] golang: aio_buffer.go: Make it safer to use

[Libguestfs] [PATCH libnbd 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()

[Libguestfs] [PATCH libnbd 6/9] golang: tests: Use AioBuffer.Slice()

[Libguestfs] [PATCH libnbd 7/9] golang: aio_buffer.go: Speed up FromBytes()