[dpdk-dev] [PATCH 07/21] vhost: add iotlb helper functions

[Maxime Coquelin]

On 09/08/2017 10:36 AM, Yuanhan Liu wrote:
> On Fri, Sep 08, 2017 at 10:24:58AM +0200, Maxime Coquelin wrote:
>>
>>
>> On 09/08/2017 10:08 AM, Yuanhan Liu wrote:
>>> On Thu, Aug 31, 2017 at 11:50:09AM +0200, Maxime Coquelin wrote:
>>>> diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
>>>> new file mode 100644
>>>> index 000000000..1b739dae5
>>>> --- /dev/null
>>>> +++ b/lib/librte_vhost/iotlb.c
>>>> @@ -0,0 +1,231 @@
>>>> +/*-
>>>> + *   BSD LICENSE
>>>> + *
>>>> + *   Copyright (c) 2017 Red Hat, Inc.
>>>> + *   Copyright (c) 2017 Maxime Coquelin <maxime.coquelin at redhat.com>
>>>
>>> I'm not a lawer, but I have been told many years before, that you don't
>>> have the copyright for the code you write for open source project, the
>>> company you work for does.
>>>
>>> Thus, it's more common to see something like following:
>>> 	Copyright , ... the commany ...
>>> 	Author:  Some One <... at ...>
>>>
>>> However, as you may have noticed, it's not common to put the authorship
>>> in the source files. Though I don't object it.
>>
>> I'm not a lawyer too. At least in other projects, it seems common the
>> author puts his name as copyright owner.
>>
>> I have no issue to change it to only keep Red Hat's one though.
> 
> That's up to you. What I said before was JFYI :)
> 
>>> [...]
>>>> +#define IOTLB_CACHE_SIZE 1024
>>>> +
>>>> +static void vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
>>>     ^^^^^^^^^^^^
>>> Note that it's not the DPDK coding style to define a function.
>>
>> Ok, I guess you mean:
>> static void
>> vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq) ?
> 
> Yep.
> 
>>>> +{
>>>> +	struct vhost_iotlb_entry *node, *temp_node;
>>>> +
>>>> +	rte_rwlock_write_lock(&vq->iotlb_lock);
>>>> +
>>>> +	TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
>>>> +		TAILQ_REMOVE(&vq->iotlb_list, node, next);
>>>> +		rte_mempool_put(vq->iotlb_pool, node);
>>>> +	}
>>>> +
>>>> +	rte_rwlock_write_unlock(&vq->iotlb_lock);
>>>> +}
>>>> +
>>>> +void vhost_user_iotlb_cache_insert(struct vhost_virtqueue *vq, uint64_t iova,
>>>> +				uint64_t uaddr, uint64_t size, uint8_t perm)
>>>> +{
>>>> +	struct vhost_iotlb_entry *node, *new_node;
>>>> +	int ret;
>>>> +
>>>> +	ret = rte_mempool_get(vq->iotlb_pool, (void **)&new_node);
>>>> +	if (ret) {
>>>> +		RTE_LOG(ERR, VHOST_CONFIG, "IOTLB pool empty, invalidate cache\n");
>>>
>>> It's a cache, why not considering remove one to get space for new one?
>>
>> It would mean having to track every lookups not to remove hot entries,
>> which would have an impact on performance.
> 
> You were removing all caches, how can we do worse than that? Even a
> random evict would be better. Or, more simply, just to remove the
> head or the tail?

I think removing head or tail could cause deadlocks.
For example it needs to translate from 0x0 to 0x2000, with page size
being 0x1000.

If cache is full, when inserting 0x1000-0x1fff, it will remove
0x0-0xfff, so a miss will be sent for 0x0-0xffff and 0x1000-0x1fff will
be remove at insert time, etc...


Note that we really need to size the cache large enough for performance
purpose, so having the cache full could be cause by either buggy or
malicious guest.


> 	--yliu
> 
>> Moreover, the idea is to have the cache large enough, else you could
>> face packet drops due to random cache misses.
>>
>> We might consider to improve it, but I consider it an optimization that
>> could be implemented later if needed.
>>
>>>> +		vhost_user_iotlb_cache_remove_all(vq);
>>>> +		ret = rte_mempool_get(vq->iotlb_pool, (void **)&new_node);
>>>> +		if (ret) {
>>>> +			RTE_LOG(ERR, VHOST_CONFIG, "IOTLB pool still empty, failure\n");
>>>> +			return;
>>>> +		}
>>>> +	}