From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tiwei.bie@intel.com>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
 by dpdk.org (Postfix) with ESMTP id 14D08293B
 for <dev@dpdk.org>; Tue,  5 Sep 2017 08:01:35 +0200 (CEST)
Received: from orsmga005.jf.intel.com ([10.7.209.41])
 by orsmga105.jf.intel.com with ESMTP; 04 Sep 2017 23:01:35 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.41,478,1498546800"; d="scan'208";a="145511125"
Received: from debian-zgviawfucg.sh.intel.com (HELO debian-ZGViaWFuCg)
 ([10.67.104.160])
 by orsmga005.jf.intel.com with ESMTP; 04 Sep 2017 23:01:33 -0700
Date: Tue, 5 Sep 2017 14:02:00 +0800
From: Tiwei Bie <tiwei.bie@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: dev@dpdk.org, yliu@fridaylinux.org, jfreiman@redhat.com, mst@redhat.com,
 vkaplans@redhat.com, jasowang@redhat.com
Message-ID: <20170905060200.GA28369@debian-ZGViaWFuCg>
References: <20170831095023.21037-1-maxime.coquelin@redhat.com>
 <20170831095023.21037-8-maxime.coquelin@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170831095023.21037-8-maxime.coquelin@redhat.com>
User-Agent: Mutt/1.7.2 (2016-11-26)
Subject: Re: [dpdk-dev] [PATCH 07/21] vhost: add iotlb helper functions
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 05 Sep 2017 06:01:36 -0000

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