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? Best regards, Ilya Maximets.