From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1D22CF04 for ; Tue, 5 Sep 2017 17:16:36 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3947BBAE2; Tue, 5 Sep 2017 15:16:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3947BBAE2 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=maxime.coquelin@redhat.com Received: from [10.36.112.29] (ovpn-112-29.ams2.redhat.com [10.36.112.29]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E4A6C679F0; Tue, 5 Sep 2017 15:16:32 +0000 (UTC) To: Tiwei Bie Cc: dev@dpdk.org, yliu@fridaylinux.org, jfreiman@redhat.com, mst@redhat.com, vkaplans@redhat.com, jasowang@redhat.com References: <20170831095023.21037-1-maxime.coquelin@redhat.com> <20170831095023.21037-8-maxime.coquelin@redhat.com> <20170905060200.GA28369@debian-ZGViaWFuCg> From: Maxime Coquelin Message-ID: Date: Tue, 5 Sep 2017 17:16:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170905060200.GA28369@debian-ZGViaWFuCg> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 05 Sep 2017 15:16:36 +0000 (UTC) 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Sep 2017 15:16:37 -0000 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 >