Libguestfs - Aug 2023 - [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array

Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for
accessing a C array as a Go slice in order to potentially benefit from
any optimizations in Go's copy() for bulk transfer of memory over
naive one-at-a-time iteration.  But that commit also acknowledged that
no benchmark timings were performed, which would have been useful to
demonstrat an actual benefit for using hack in the first place.  And
since we are copying data anyways (rather than using the slice to
avoid a copy), and network transmission costs have a higher impact to
performance than in-memory copying speed, it's hard to justify keeping
the hack without hard data.

What's more, while using Go's copy() on an array of C uint32_t makes
sense for 32-bit extents, our corresponding 64-bit code uses a struct
which does not map as nicely to Go's copy().  Using a common style
between both list copying helpers is beneficial to mainenance.

Additionally, at face value, converting C.size_t to int may truncate;
we could avoid that risk if we were to uniformly use uint64 instead of
int.  But we can equally just panic if the count is oversized: our
state machine guarantees that the server's response fits within 64M
bytes (count will be smaller than that, since it is multiple bytes per
extent entry).

Suggested-by: Laszlo Ersek <lersek at redhat.com>
CC: Nir Soffer <nsoffer at redhat.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---

v4: new patch to the series, but previously posted as part of the
golang cleanups.  Since then: rework the commit message as it is no
longer a true revert, and add a panic() if count exceeds expected
bounds.
---
 generator/GoLang.ml | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 73df5254..cc7d78b6 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -516,11 +516,16 @@ let
 /* Closures. */

 func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
+    if (uint64(count) > 64*1024*1024) {
+        panic(\"violation of state machine guarantee\")
+    }
     ret := make([]uint32, int(count))
-    // See
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
-    // TODO: Use unsafe.Slice() when we require Go 1.17.
-    s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]
-    copy(ret, s)
+    addr := uintptr(unsafe.Pointer(entries))
+    for i := 0; i < int(count); i++ {
+        ptr := (*C.uint32_t)(unsafe.Pointer(addr))
+        ret[i] = uint32(*ptr)
+        addr += unsafe.Sizeof(*ptr)
+    }
     return ret
 }
 ";
-- 
2.41.0

On Wed, Aug 02, 2023 at 08:50:25PM -0500, Eric Blake
wrote:
> Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for
> accessing a C array as a Go slice in order to potentially benefit from
> any optimizations in Go's copy() for bulk transfer of memory over
> naive one-at-a-time iteration.  But that commit also acknowledged that
> no benchmark timings were performed, which would have been useful to
> demonstrat an actual benefit for using hack in the first place.  And
"demonstrate"
> since we are copying data anyways (rather than using the slice to
> avoid a copy), and network transmission costs have a higher impact to
> performance than in-memory copying speed, it's hard to justify keeping
> the hack without hard data.
> 
> What's more, while using Go's copy() on an array of C uint32_t
makes
> sense for 32-bit extents, our corresponding 64-bit code uses a struct
> which does not map as nicely to Go's copy().  Using a common style
> between both list copying helpers is beneficial to mainenance.
> 
> Additionally, at face value, converting C.size_t to int may truncate;
> we could avoid that risk if we were to uniformly use uint64 instead of
> int.  But we can equally just panic if the count is oversized: our
> state machine guarantees that the server's response fits within 64M
> bytes (count will be smaller than that, since it is multiple bytes per
> extent entry).
> 
> Suggested-by: Laszlo Ersek <lersek at redhat.com>
> CC: Nir Soffer <nsoffer at redhat.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> v4: new patch to the series, but previously posted as part of the
> golang cleanups.  Since then: rework the commit message as it is no
> longer a true revert, and add a panic() if count exceeds expected
> bounds.
> ---
>  generator/GoLang.ml | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 73df5254..cc7d78b6 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -516,11 +516,16 @@ let
>  /* Closures. */
> 
>  func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
> +    if (uint64(count) > 64*1024*1024) {
> +        panic(\"violation of state machine guarantee\")
> +    }
Not sure if golang includes the line number when it panics, but a
brief bit of explanation might help here.  Something like:

  panic(\"internal error: state machine should guarantee <= 64M of data
\
          in the extents reply, but this was exceeded unexpectedly\")
>      ret := make([]uint32, int(count))
> -    // See
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
> -    // TODO: Use unsafe.Slice() when we require Go 1.17.
> -    s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]
The whole magical 1 << 30 was another thing to dislike about golang,
so glad to see it gone here.
> -    copy(ret, s)
> +    addr := uintptr(unsafe.Pointer(entries))
> +    for i := 0; i < int(count); i++ {
> +        ptr := (*C.uint32_t)(unsafe.Pointer(addr))
> +        ret[i] = uint32(*ptr)
> +        addr += unsafe.Sizeof(*ptr)
> +    }
>      return ret
>  }
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Thu, Aug 3, 2023 at 4:57?AM Eric Blake <eblake at redhat.com>
wrote:
>
> Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for
> accessing a C array as a Go slice in order to potentially benefit from
> any optimizations in Go's copy() for bulk transfer of memory over
> naive one-at-a-time iteration.  But that commit also acknowledged that
> no benchmark timings were performed, which would have been useful to
> demonstrat an actual benefit for using hack in the first place.  And
Why do you call this a hack? This is the documented way to create a Go
slice from memory.
> since we are copying data anyways (rather than using the slice to
> avoid a copy), and network transmission costs have a higher impact to
> performance than in-memory copying speed, it's hard to justify keeping
> the hack without hard data.
Since this is not a hack we don't need to justify it :-)
> What's more, while using Go's copy() on an array of C uint32_t
makes
> sense for 32-bit extents, our corresponding 64-bit code uses a struct
> which does not map as nicely to Go's copy().
If we return a slice of the C extent type, copy() can work, but it is probably
not what we want to return.
> Using a common style
> between both list copying helpers is beneficial to mainenance.
>
> Additionally, at face value, converting C.size_t to int may truncate;
> we could avoid that risk if we were to uniformly use uint64 instead of
> int.  But we can equally just panic if the count is oversized: our
> state machine guarantees that the server's response fits within 64M
> bytes (count will be smaller than that, since it is multiple bytes per
> extent entry).
Good to check this, but not related to changing the way we copy the array.
> Suggested-by: Laszlo Ersek <lersek at redhat.com>
> CC: Nir Soffer <nsoffer at redhat.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>
> v4: new patch to the series, but previously posted as part of the
> golang cleanups.  Since then: rework the commit message as it is no
> longer a true revert, and add a panic() if count exceeds expected
> bounds.
> ---
>  generator/GoLang.ml | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 73df5254..cc7d78b6 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -516,11 +516,16 @@ let
>  /* Closures. */
>
>  func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
> +    if (uint64(count) > 64*1024*1024) {
> +        panic(\"violation of state machine guarantee\")
This is unwanted in a library, it means the entire application will crash
because of a bug in the library. Can we convert this to an error in the caller?
> +    }
>      ret := make([]uint32, int(count))
> -    // See
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
> -    // TODO: Use unsafe.Slice() when we require Go 1.17.
> -    s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]
Can we require Go 1.17? (current version is 1.20)

In Go >= 1.17, we can use something like:

    s := unsafe.Slice(C.uint32_t, length)
> -    copy(ret, s)
> +    addr := uintptr(unsafe.Pointer(entries))
> +    for i := 0; i < int(count); i++ {
> +        ptr := (*C.uint32_t)(unsafe.Pointer(addr))
> +        ret[i] = uint32(*ptr)
> +        addr += unsafe.Sizeof(*ptr)
> +    }
This loop is worse than the ugly line creating a slice.
With a slice we can do:

    for i, item := range s {
        ret[i] = uint32(item)
    }

(I did not try to compile this)