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

Maxime Coquelin maxime.coquelin at redhat.com
Tue Sep 5 17:16:29 CEST 2017



On 09/05/2017 08:02 AM, Tiwei Bie wrote:
> On Thu, Aug 31, 2017 at 11:50:09AM +0200, Maxime Coquelin wrote:
> [...]
>> +
>> +#define IOTLB_CACHE_SIZE 1024
>> +
>> +static void vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
>> +{
>> +	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");
> 
> I think the log level should be DEBUG or INFO or the likes, not ERR.

I set it to ERR because failing in this case would mean that either the 
IOTLB cache has not been sized large enough and we would like it to be
reported as it would have an impact on performance and drop rate, or
that we have a malicious or buggy guest.

I'm fine with changing it to INFO though.

>> +		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;
>> +		}
>> +	}
>> +
> [...]
>> +
>> +void vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
>> +					uint64_t iova, uint64_t size)
>> +{
>> +	struct vhost_iotlb_entry *node, *temp_node;
>> +
>> +	if (unlikely(!size))
>> +		return;
>> +
>> +	rte_rwlock_write_lock(&vq->iotlb_lock);
>> +
>> +	TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
>> +		/* Sorted list */
>> +		if (unlikely(node->iova >= iova + size)) {
>> +			break;
>> +		} else if ((node->iova < iova + size) &&
>> +					(iova < node->iova + node->size)) {
> 
> The `else' can be removed.
> And the check of (node->iova < iova + size) can also be removed.

Right! Fixed.

>> +			TAILQ_REMOVE(&vq->iotlb_list, node, next);
>> +			rte_mempool_put(vq->iotlb_pool, node);
>> +			continue;
>> +		}
>> +	}
>> +
>> +	rte_rwlock_write_unlock(&vq->iotlb_lock);
>> +}
>> +
>> +
> 
> Only one empty line is needed here.

Fixed.

>> +uint64_t vhost_user_iotlb_cache_find(struct vhost_virtqueue *vq, uint64_t iova,
>> +						uint64_t *size, uint8_t perm)
>> +{
> [...]
>> +#ifndef _VHOST_IOTLB_H_
>> +#define _VHOST_IOTLB_H_
>> +
>> +#include "vhost.h"
>> +void vhost_user_iotlb_cache_insert(struct vhost_virtqueue *vq, uint64_t iova,
>> +					uint64_t uaddr, uint64_t size,
>> +					uint8_t perm);
> 
> An empty line should be added after #include "vhost.h".
Fixed.

Thanks,
Maxime

> Best regards,
> Tiwei Bie
> 


More information about the dev mailing list