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

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)

On Tue, Aug 08, 2023 at 02:36:12PM +0300, Nir Soffer
wrote:
> On Thu, Aug 3, 2023 at 4:57?AM Eric Blake <eblake at redhat.com>
wrote:
> >  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?
In Eric's defence I want to point out that this is (supposed to be) a
"should never happen" internal error, like an assert in C.  What's
the
proper way to handle an internal error?
> > +    }
> >      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)
Golang 1.17 was released in Aug 2021 (2 years ago) which is still
fairly recent.

On the other hand, RHEL 8 has 1.19 for some reason -- RHEL 8 seem to
rebasing golang like crazy.

So I guess this would be fine.
> > -    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)
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 8/8/23 13:36, Nir Soffer wrote:
> 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.
Just because it is documented, it's not less terrible.

<https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices>:

% use a type conversion to a pointer to a very big array and then slice
% it to the length that you want

The "very big array" is the hack.

The documentation uses 1 << 28 for the element count, the libnbd code
uses 1 << 30 as the element count. In libnbd, the element type is
uint32, so we're banking on (a) the 4G size (byte count) being safely
expressible for the fake array, (b) the 1G elements in the fake array
sufficing to cover the actual C array we want.

In the documentation, the 1<<28 element count makes the same two
assumptions, which is equally bad for (b) (who knows if the actual C
array has no more than 256M elements), and *worse* for (a) (because
"C.YourType" could be a 128 byte large structure, and then the byte
count in the fake array would be 32G -- not representable in 32 bits.
Which may or may not matter).

In other words, the documented workaround is full of uncomfortable
assumptions that make anyone tired *if* the reader even bothers to
enumerate those assumptions.
>> 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 :-)
It's a hack because it contains an unjustifiable and/or risky open-coded
constant, namely 1 << 28 (or 1 << 30, in libnbd).
>
>> 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?
It's actually an assert().

As long as you agree that assertions are allowed in a library (for
expressing invariants ensured by other parts of the library), this panic
is not functionally wrong.

I'm unsure if "base Go" has something that is spelled
"assert".
According to
<https://stackoverflow.com/questions/47558389/what-is-the-go-equivalent-to-assert-in-c>,
there isn't.

(BTW my original recommendation here was a loop that would not rely on
this invariant (instead it would use uint64 for "count"). But
asserting
the invariant is fine as well, IMO.)

I don't think it's a good approach to simply return an error if internal
inconsistency is detected in a library. How is the application supposed
to handle that / recover from the problem? The internal state is already
busted, continuing beyond that point could only do more damage.

The Go language docs indeed describe how the Go designers think about
assertions <https://go.dev/doc/faq#assertions>:

% Go doesn't provide assertions. They are undeniably convenient, but our
% experience has been that programmers use them as a crutch to avoid
% thinking about proper error handling and reporting. Proper error
% handling means that servers continue to operate instead of crashing
% after a non-fatal error. Proper error reporting means that errors are
% direct and to the point, saving the programmer from interpreting a
% large crash trace. Precise errors are particularly important when the
% programmer seeing the errors is not familiar with the code.
%
% We understand that this is a point of contention. There are many
% things in the Go language and libraries that differ from modern
% practices, simply because we feel it's sometimes worth trying a
% different approach.

I *have* seen the abuse they describe there (very frequently at that),
and I absolutely agree that using assert() for copping out of explicit
error checking is intolerable. But Go's approach is overkill; it goes to
the other extreme. Valid ways to use assert() *remain*.

The assertion in this particular instance is *not* a "crutch to avoid
thinking about proper error handling and reporting". There's nothing to
handle here. If this condition fails, that's a programming bug in
libnbd, and we're best served by a coredump. It *is* a fatal error.
>
>> +    }
>>      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)
I agree that this could be the best.

When I proposed the open-coded loop, I was aware of the Go-1.17
reference above, but I didn't know if we wanted to require Go-1.17, and
I didn't look further myself.

Checking now... The hack was originally written in January 2022. Go-1.17
was released in August 2021, so only 5 months earlier. Go-1.17 was
probably too fresh at the time for libnbd to require it everywhere.

However, nowadays even RHEL7 provides Go 1.19 -- 1.19.6 to be precise,
in DevToolSet 2023.2:

https://access.redhat.com/support/policy/updates/red-hat-developer-tools
>
>> -    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.
I disagree :) It's explicit, well-defined, has no "ballpark"
constants.
Any performance benefit of the hack had not been shown. And we're going
to copy the data anyway, later.
> With a slice we can do:
>
>     for i, item := range s {
>         ret[i] = uint32(item)
>     }
>
> (I did not try to compile this)
>
Laszlo