On Fri, Aug 11, 2023, 08:50 Ilya Maximets <i.maxim...@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.maxim...@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 = NULL instead of touching all bytes
> > of the struct like = MEMORY_REGION_CACHE_INVALID. There would be less
> > code and the existing 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.
Stefan
>
