Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors

[Ilya Maximets]

On 8/11/23 15:58, Stefan Hajnoczi wrote:
> 
> 
> On Fri, Aug 11, 2023, 08:50 Ilya Maximets <i.maximets@ovn.org 
> <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 8/10/23 17:50, Stefan Hajnoczi wrote:
>     > On Tue, Aug 08, 2023 at 12:28:47AM +0200, Ilya Maximets wrote:
>     >> Lots of virtio functions that are on a hot path in data transmission
>     >> are initializing indirect descriptor cache at the point of stack
>     >> allocation.  It's a 112 byte structure that is getting zeroed out on
>     >> each call adding unnecessary overhead.  It's going to be correctly
>     >> initialized later via special init function.  The only reason to
>     >> actually initialize right away is the ability to safely destruct it.
>     >> However, we only need to destruct it when it was used, i.e. when a
>     >> desc_cache points to it.
>     >>
>     >> Removing these unnecessary stack initializations improves throughput
>     >> of virtio-net devices in terms of 64B packets per second by 6-14 %
>     >> depending on the case.  Tested with a proposed af-xdp network backend
>     >> and a dpdk testpmd application in the guest, but should be beneficial
>     >> for other virtio devices as well.
>     >>
>     >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org 
> <mailto:i.maximets@ovn.org>>
>     >> ---
>     >>  hw/virtio/virtio.c | 42 +++++++++++++++++++++++++++---------------
>     >>  1 file changed, 27 insertions(+), 15 deletions(-)
>     >
>     > Another option is to create an address_space_cache_init_invalid()
>     > function that only assigns mrs.mr <http://mrs.mr> = NULL instead of 
> touching all bytes
>     > of the struct like = MEMORY_REGION_CACHE_INVALID. There would be less
>     > code and the existing mrs.mr <http://mrs.mr> check in 
> address_space_cache_destroy()
>     > would serve the same function as the desc_cache == &indirect_desc_cache
>     > check added by this patch.
> 
>     It does look simpler this way, indeed.  Though I'm not sure about
>     a function name.  We have address_space_cache_invalidate() that
>     does a completely different thing and the invalidated cache can
>     still be used, while the cache initialized with the newly proposed
>     address_space_cache_init_invalid() can not be safely used.
> 
>     I suppose, the problem is not new, since the macro was named similarly,
>     but making it a function seems to make the issue worse.
> 
>     Maybe address_space_cache_init_empty() will be a better name?
>     E.g.:
> 
>     **
>      * address_space_cache_init_empty: Initialize empty #MemoryRegionCache
>      *
>      * @cache: The #MemoryRegionCache to operate on.
>      *
>      * Initializes #MemoryRegionCache structure without memory region 
> attached.
>      * Cache initialized this way can only be safely destroyed, but not used.
>      */
>     static inline void address_space_cache_init_empty(MemoryRegionCache 
> *cache)
>     {
>         cache->mrs.mr = NULL;
>     }
> 
>     What do you think?
> 
> 
> init_empty() is good.

I'll use it then.  Will send a v2 shortly.

Thanks!

> 
> Stefan
>