[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtio: don't zero out memory region cache for indirect desc
From: |
Ilya Maximets |
Subject: |
Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors |
Date: |
Fri, 11 Aug 2023 14:51:00 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 |
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>
>> ---
>> 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.