| Message ID | 20151109175834.4076.53346.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 09 Nov 2015 18:58:34 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro > and the virtio_access_is_big_endian() helper to have a branchless fast path > in the virtio memory accessors for targets that don't switch endian. > > This was considered as a strong requirement at the time. > > Now we have added a runtime check for virtio 1.0, which ruins the benefit > of the virtio_access_is_big_endian() helper for always little-endian targets. > > With this patch, fixed little-endian targets stop checking for virtio 1.0, > since the result is little-endian in all cases. So always-LE gets optimized, while always-BE and bi-endian stay the same? (Is there a measurable impact?) > The helper also gets renamed > so it is clear it is optimized for fast paths. Even if it isn't actually 'fast' on anything other than fixed-LE? > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 20 deletions(-)
On Thu, 12 Nov 2015 19:08:59 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Mon, 09 Nov 2015 18:58:34 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro > > and the virtio_access_is_big_endian() helper to have a branchless fast path > > in the virtio memory accessors for targets that don't switch endian. > > > > This was considered as a strong requirement at the time. > > > > Now we have added a runtime check for virtio 1.0, which ruins the benefit > > of the virtio_access_is_big_endian() helper for always little-endian targets. > > > > With this patch, fixed little-endian targets stop checking for virtio 1.0, > > since the result is little-endian in all cases. > > So always-LE gets optimized, while always-BE and bi-endian stay the same? > Yes. > (Is there a measurable impact?) > I tried to measure using iperf between host and guest but I could not find any significant change... do you think about another test I could try ? > > The helper also gets renamed > > so it is clear it is optimized for fast paths. > > Even if it isn't actually 'fast' on anything other than fixed-LE? Yes this is definitely a fixed-LE only optimization... should I drop the name change and add a comment instead ? > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 20 deletions(-)
On Fri, 13 Nov 2015 10:28:31 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Thu, 12 Nov 2015 19:08:59 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Mon, 09 Nov 2015 18:58:34 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro > > > and the virtio_access_is_big_endian() helper to have a branchless fast path > > > in the virtio memory accessors for targets that don't switch endian. > > > > > > This was considered as a strong requirement at the time. > > > > > > Now we have added a runtime check for virtio 1.0, which ruins the benefit > > > of the virtio_access_is_big_endian() helper for always little-endian targets. > > > > > > With this patch, fixed little-endian targets stop checking for virtio 1.0, > > > since the result is little-endian in all cases. > > > > So always-LE gets optimized, while always-BE and bi-endian stay the same? > > > > Yes. > > > (Is there a measurable impact?) > > > > I tried to measure using iperf between host and guest but I could not > find any significant change... do you think about another test I could > try ? My hunch is that the impact is small anyway. > > > > The helper also gets renamed > > > so it is clear it is optimized for fast paths. > > > > Even if it isn't actually 'fast' on anything other than fixed-LE? > > Yes this is definitely a fixed-LE only optimization... should I drop the name > change and add a comment instead ? I think that would be better as it does not raise expectations :) > > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > --- > > > include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++----------------- > > > 1 file changed, 23 insertions(+), 20 deletions(-) >
On Fri, 13 Nov 2015 15:42:53 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Fri, 13 Nov 2015 10:28:31 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Thu, 12 Nov 2015 19:08:59 +0100 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > On Mon, 09 Nov 2015 18:58:34 +0100 > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > > > When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro > > > > and the virtio_access_is_big_endian() helper to have a branchless fast path > > > > in the virtio memory accessors for targets that don't switch endian. > > > > > > > > This was considered as a strong requirement at the time. > > > > > > > > Now we have added a runtime check for virtio 1.0, which ruins the benefit > > > > of the virtio_access_is_big_endian() helper for always little-endian targets. > > > > > > > > With this patch, fixed little-endian targets stop checking for virtio 1.0, > > > > since the result is little-endian in all cases. > > > > > > So always-LE gets optimized, while always-BE and bi-endian stay the same? > > > > > > > Yes. > > > > > (Is there a measurable impact?) > > > > > > > I tried to measure using iperf between host and guest but I could not > > find any significant change... do you think about another test I could > > try ? > > My hunch is that the impact is small anyway. > Yeah... even when running TCG I don't see any difference. > > > > > > The helper also gets renamed > > > > so it is clear it is optimized for fast paths. > > > > > > Even if it isn't actually 'fast' on anything other than fixed-LE? > > > > Yes this is definitely a fixed-LE only optimization... should I drop the name > > change and add a comment instead ? > > I think that would be better as it does not raise expectations :) > Ok I'll revert to the original name then. Thanks for the review. -- Greg > > > > > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > --- > > > > include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++----------------- > > > > 1 file changed, 23 insertions(+), 20 deletions(-) > > >
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h index ba1530619939..ff013519b9dc 100644 --- a/include/hw/virtio/virtio-access.h +++ b/include/hw/virtio/virtio-access.h @@ -17,12 +17,15 @@ #include "hw/virtio/virtio.h" #include "exec/address-spaces.h" -static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) +static inline bool virtio_is_big_endian_fast(VirtIODevice *vdev) { +#if defined(TARGET_IS_BIENDIAN) || defined(TARGET_WORDS_BIGENDIAN) if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { /* Devices conforming to VIRTIO 1.0 or later are always LE. */ return false; } +#endif + #if defined(TARGET_IS_BIENDIAN) return virtio_is_big_endian(vdev); #elif defined(TARGET_WORDS_BIGENDIAN) @@ -34,7 +37,7 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return lduw_be_phys(&address_space_memory, pa); } return lduw_le_phys(&address_space_memory, pa); @@ -42,7 +45,7 @@ static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa) static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return ldl_be_phys(&address_space_memory, pa); } return ldl_le_phys(&address_space_memory, pa); @@ -50,7 +53,7 @@ static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa) static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return ldq_be_phys(&address_space_memory, pa); } return ldq_le_phys(&address_space_memory, pa); @@ -59,7 +62,7 @@ static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa) static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa, uint16_t value) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { stw_be_phys(&address_space_memory, pa, value); } else { stw_le_phys(&address_space_memory, pa, value); @@ -69,7 +72,7 @@ static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa, static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa, uint32_t value) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { stl_be_phys(&address_space_memory, pa, value); } else { stl_le_phys(&address_space_memory, pa, value); @@ -78,7 +81,7 @@ static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa, static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { stw_be_p(ptr, v); } else { stw_le_p(ptr, v); @@ -87,7 +90,7 @@ static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v) static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { stl_be_p(ptr, v); } else { stl_le_p(ptr, v); @@ -96,7 +99,7 @@ static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v) static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { stq_be_p(ptr, v); } else { stq_le_p(ptr, v); @@ -105,7 +108,7 @@ static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v) static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return lduw_be_p(ptr); } else { return lduw_le_p(ptr); @@ -114,7 +117,7 @@ static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr) static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return ldl_be_p(ptr); } else { return ldl_le_p(ptr); @@ -123,7 +126,7 @@ static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr) static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr) { - if (virtio_access_is_big_endian(vdev)) { + if (virtio_is_big_endian_fast(vdev)) { return ldq_be_p(ptr); } else { return ldq_le_p(ptr); @@ -133,18 +136,18 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr) static inline bool virtio_needs_swap(VirtIODevice *vdev) { #ifdef HOST_WORDS_BIGENDIAN - return virtio_access_is_big_endian(vdev) ? false : true; + return virtio_is_big_endian_fast(vdev) ? false : true; #else - return virtio_access_is_big_endian(vdev) ? true : false; + return virtio_is_big_endian_fast(vdev) ? true : false; #endif } static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s) { #ifdef HOST_WORDS_BIGENDIAN - return virtio_access_is_big_endian(vdev) ? s : bswap16(s); + return virtio_is_big_endian_fast(vdev) ? s : bswap16(s); #else - return virtio_access_is_big_endian(vdev) ? bswap16(s) : s; + return virtio_is_big_endian_fast(vdev) ? bswap16(s) : s; #endif } @@ -156,9 +159,9 @@ static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s) static inline uint32_t virtio_tswap32(VirtIODevice *vdev, uint32_t s) { #ifdef HOST_WORDS_BIGENDIAN - return virtio_access_is_big_endian(vdev) ? s : bswap32(s); + return virtio_is_big_endian_fast(vdev) ? s : bswap32(s); #else - return virtio_access_is_big_endian(vdev) ? bswap32(s) : s; + return virtio_is_big_endian_fast(vdev) ? bswap32(s) : s; #endif } @@ -170,9 +173,9 @@ static inline void virtio_tswap32s(VirtIODevice *vdev, uint32_t *s) static inline uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s) { #ifdef HOST_WORDS_BIGENDIAN - return virtio_access_is_big_endian(vdev) ? s : bswap64(s); + return virtio_is_big_endian_fast(vdev) ? s : bswap64(s); #else - return virtio_access_is_big_endian(vdev) ? bswap64(s) : s; + return virtio_is_big_endian_fast(vdev) ? bswap64(s) : s; #endif }
When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro and the virtio_access_is_big_endian() helper to have a branchless fast path in the virtio memory accessors for targets that don't switch endian. This was considered as a strong requirement at the time. Now we have added a runtime check for virtio 1.0, which ruins the benefit of the virtio_access_is_big_endian() helper for always little-endian targets. With this patch, fixed little-endian targets stop checking for virtio 1.0, since the result is little-endian in all cases. The helper also gets renamed so it is clear it is optimized for fast paths. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- include/hw/virtio/virtio-access.h | 43 ++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 20 deletions(-)