On 8/11/23 15:58, Stefan Hajnoczi wrote: > > > On Fri, Aug 11, 2023, 08:50 Ilya Maximets <i.maxim...@ovn.org > <mailto: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 > <mailto: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 <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 >
