From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58395) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIjyo-00053m-Tm for qemu-devel@nongnu.org; Thu, 21 Mar 2013 14:12:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIjyg-0004Ea-SN for qemu-devel@nongnu.org; Thu, 21 Mar 2013 14:12:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIjyg-0004ED-Jj for qemu-devel@nongnu.org; Thu, 21 Mar 2013 14:12:38 -0400 Message-ID: <514B4D90.7080007@redhat.com> Date: Thu, 21 Mar 2013 12:12:32 -0600 From: Eric Blake MIME-Version: 1.0 References: <1363881457-14814-1-git-send-email-pl@kamp.de> <1363881457-14814-3-git-send-email-pl@kamp.de> In-Reply-To: <1363881457-14814-3-git-send-email-pl@kamp.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2XHFAURQXJAECECVCKANL" Subject: Re: [Qemu-devel] [PATCHv3 2/9] cutils: add a function to find non-zero content in a buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2XHFAURQXJAECECVCKANL Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: Peter Lieven > --- > 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) > } > =20 > /* > + * Searches for an area with non-zero content in a buffer > + * > + * Attention! The len must be a multiple of 8 * sizeof(VECTYPE)=20 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=20 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. > + *=20 > + * can_use_buffer_find_nonzero_offset() can be used to check > + * these requirements. > + *=20 > + * The return value is the offset of the non-zero area rounded > + * down to 8 * sizeof(VECTYPE). If the buffer is all zero=20 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 =3D (VECTYPE *)buf; > + VECTYPE zero =3D ZERO_SPLAT; > + size_t i; > + =20 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 >=20 Cleaning up whitespace is trivial; but the incorrect use of 'inline' requires a v4. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2XHFAURQXJAECECVCKANL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRS02QAAoJEKeha0olJ0Nqum4H/2AYIjmrMQZiivTpcG6nvS+v ms9RVws9f6HSF9h7ckZRyIMgW2hZD8QoZMCo17tJxK/3aatcaxqe2m9a0QwDloI4 UPkplytRQZpOfbDlJuMBCjPBFwZC2IKxA2pxfItLWzWXH+MBmUjncdR7Th3GNW7F mk5xVgV4VAJarwIgRgb8xnSfl1xo1sZ90WnO+AQHlipfjnoVCWR1T5W9Wb0QxV5U C+qzxesU6B+eujwSF8leKxQUO76sNPdnM16qEzgBT7JnnZ0lnuTEsureg9e+l+LX WBAaTuIEjks33byrTozUkgAHX7aU0YnPaRlm6azR/MVMoTKG3kyxZtGcXeJ3R+o= =aogJ -----END PGP SIGNATURE----- ------enig2XHFAURQXJAECECVCKANL--