Re: [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets

[Greg Kurz]

On Fri, 13 Nov 2015 15:42:53 +0100
Cornelia Huck <address@hidden> wrote:

> On Fri, 13 Nov 2015 10:28:31 +0100
> Greg Kurz <address@hidden> wrote:
> 
> > On Thu, 12 Nov 2015 19:08:59 +0100
> > Cornelia Huck <address@hidden> wrote:
> > 
> > > On Mon, 09 Nov 2015 18:58:34 +0100
> > > Greg Kurz <address@hidden> 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 <address@hidden>
> > > > ---
> > > >  include/hw/virtio/virtio-access.h |   43 
> > > > ++++++++++++++++++++-----------------
> > > >  1 file changed, 23 insertions(+), 20 deletions(-)
> > 
>