Re: [Qemu-devel] [PATCHv3 2/9] cutils: add a function to find non-zero content in a buffer

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3049 bytes --]

On 03/21/2013 09:57 AM, Peter Lieven wrote:
> this adds buffer_find_nonzero_offset() which is a SSE2/Altives

s/Altives/Altivec/

> optimized function that searches for non-zero content in a
> buffer.
> 
> due to the optimizations used in the function there are restrictions
> on buffer address and search length. the function
> can_use_buffer_find_nonzero_content() can be used to check if
> the function can be used safely.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/qemu-common.h |    3 +++
>  util/cutils.c         |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)

> +inline bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len);
> +inline size_t buffer_find_nonzero_offset(const void *buf, size_t len);

Ouch.  It is okay to add a 'static inline' function, but then the
implementation must live in this header.  Otherwise, the function must
not be inline, or you risk linker errors.

> +++ b/util/cutils.c
> @@ -143,6 +143,56 @@ int qemu_fdatasync(int fd)
>  }
>  
>  /*
> + * Searches for an area with non-zero content in a buffer
> + *
> + * Attention! The len must be a multiple of 8 * sizeof(VECTYPE) 

Should we call out BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR instead of a
magic number here?  But I'm okay with leaving it as-is.

> + * and addr must be a multiple of sizeof(VECTYPE) due to 

Trailing whitespace (here, and on several other lines).  Please run your
series through scripts/checkpatch.pl before submitting v4.

> + * restriction of optimizations in this function.
> + * 
> + * can_use_buffer_find_nonzero_offset() can be used to check
> + * these requirements.
> + * 
> + * The return value is the offset of the non-zero area rounded
> + * down to 8 * sizeof(VECTYPE). If the buffer is all zero 

Same comment on this use of '8'.

> + * the return value is equal to len.
> + */
> +
> +inline size_t buffer_find_nonzero_offset(const void *buf, size_t len)

s/inline// (or move it to a 'static inline' definition in the .h)

> +{
> +    VECTYPE *p = (VECTYPE *)buf;
> +    VECTYPE zero = ZERO_SPLAT;
> +    size_t i;
> +    

You copied the 'Attention! ...' message from buffer_is_zero, which
currently asserts that its condition is held.  Therefore, consistency
would argue that you should assert your preconditions here, even if it
adds more to the code size.  But this is something where a maintainer
might have a better opinion on whether to keep the code robust with an
assert(), or whether the faster operation without sanity checking is
more appropriate (in which case a followup to remove the assert from
buffer_is_zero would make sense).

>   * Checks if a buffer is all zeroes
>   *
>   * Attention! The len must be a multiple of 4 * sizeof(long) due to
> 

Cleaning up whitespace is trivial; but the incorrect use of 'inline'
requires a v4.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]