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

[Tiwei Bie <tiwei.bie@intel.com>]

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.

> +		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.

> +			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.

> +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".

Best regards,
Tiwei Bie