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

[Maxime Coquelin]

On 09/08/2017 11:21 AM, Yuanhan Liu wrote:
> On Fri, Sep 08, 2017 at 10:50:49AM +0200, Maxime Coquelin wrote:
>>>>>> +{
>>>>>> +	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...
> 
> Okay, that means we can't simply remove the head or tail.
> 
>> 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.
> 
> I agree. But for the malicious guest, it could lead to a DDOS attack:
> assume it keeps vhost running out of cache and then vhost keeps printing
> above message.
> 
> What I suggested was to evict one (by some polices) to get a space for
> the new one we want to insert. Note that it won't be a performance issue,
> IMO, due to we only do that when we run out of caches, which, according
> to your sayings, should not happen in normal cases.

Ok, so let's randomly remove one entry. What about using something like:
rte_rand() % IOTLB_CACHE_SIZE

> 
> 	--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;
>>>>>> +		}
>>>>>> +	}