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 1EEB42BFE for ; Fri, 8 Sep 2017 10:25:10 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 674EFC0587EB; Fri, 8 Sep 2017 08:25:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 674EFC0587EB Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=maxime.coquelin@redhat.com Received: from [10.36.112.16] (ovpn-112-16.ams2.redhat.com [10.36.112.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 522C060A9B; Fri, 8 Sep 2017 08:25:01 +0000 (UTC) To: Yuanhan Liu Cc: dev@dpdk.org, jfreiman@redhat.com, tiwei.bie@intel.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> <20170908080855.GF9736@yliu-home> From: Maxime Coquelin Message-ID: <8a9b486b-635a-ec70-76f9-de830ad21882@redhat.com> Date: Fri, 8 Sep 2017 10:24:58 +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: <20170908080855.GF9736@yliu-home> 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.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 08 Sep 2017 08:25:09 +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: Fri, 08 Sep 2017 08:25:10 -0000 On 09/08/2017 10:08 AM, Yuanhan Liu wrote: > On Thu, Aug 31, 2017 at 11:50:09AM +0200, Maxime Coquelin wrote: >> diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c >> new file mode 100644 >> index 000000000..1b739dae5 >> --- /dev/null >> +++ b/lib/librte_vhost/iotlb.c >> @@ -0,0 +1,231 @@ >> +/*- >> + * BSD LICENSE >> + * >> + * Copyright (c) 2017 Red Hat, Inc. >> + * Copyright (c) 2017 Maxime Coquelin > > I'm not a lawer, but I have been told many years before, that you don't > have the copyright for the code you write for open source project, the > company you work for does. > > Thus, it's more common to see something like following: > Copyright , ... the commany ... > Author: Some One <...@...> > > However, as you may have noticed, it's not common to put the authorship > in the source files. Though I don't object it. I'm not a lawyer too. At least in other projects, it seems common the author puts his name as copyright owner. I have no issue to change it to only keep Red Hat's one though. > [...] >> +#define IOTLB_CACHE_SIZE 1024 >> + >> +static void vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq) > ^^^^^^^^^^^^ > Note that it's not the DPDK coding style to define a function. Ok, I guess you mean: 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"); > > 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. 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; >> + } >> + } >> + >> + new_node->iova = iova; >> + new_node->uaddr = uaddr; >> + new_node->size = size; >> + new_node->perm = perm; >> + >> + rte_rwlock_write_lock(&vq->iotlb_lock); >> + >> + TAILQ_FOREACH(node, &vq->iotlb_list, next) { >> + /* >> + * Entries must be invalidated before being updated. >> + * So if iova already in list, assume identical. >> + */ >> + if (node->iova == new_node->iova) { >> + rte_mempool_put(vq->iotlb_pool, new_node); >> + goto unlock; >> + } else if (node->iova > new_node->iova) { >> + TAILQ_INSERT_BEFORE(node, new_node, next); >> + goto unlock; >> + } >> + } >> + >> + TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next); >> + >> +unlock: >> + rte_rwlock_write_unlock(&vq->iotlb_lock); >> +} >> + >> +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 */ > > I'd like to put such comments at the struct declartion, so that you don't > have to mention it many times that it's a sorted list. Ok, I'll comment directly in struct declaration. >> + if (unlikely(node->iova >= iova + size)) { >> + break; >> + } else if ((node->iova < iova + size) && >> + (iova < node->iova + node->size)) { >> + TAILQ_REMOVE(&vq->iotlb_list, node, next); >> + rte_mempool_put(vq->iotlb_pool, node); >> + continue; >> + } >> + } >> + >> + rte_rwlock_write_unlock(&vq->iotlb_lock); >> +} >> + > [...] >> +int vhost_user_iotlb_init(struct virtio_net *dev, int vq_index) >> +{ >> + char pool_name[RTE_MEMPOOL_NAMESIZE]; >> + struct vhost_virtqueue *vq = dev->virtqueue[vq_index]; >> + int ret = -1, socket; >> + >> + if (vq->iotlb_pool) { >> + /* >> + * The cache has already been initialized, >> + * just drop all entries >> + */ >> + vhost_user_iotlb_cache_remove_all(vq); >> + return 0; >> + } >> + >> +#ifdef RTE_LIBRTE_VHOST_NUMA >> + ret = get_mempolicy(&socket, NULL, 0, vq, MPOL_F_NODE | MPOL_F_ADDR); >> +#endif >> + if (ret) >> + socket = 0; >> + >> + rte_rwlock_init(&vq->iotlb_lock); >> + >> + TAILQ_INIT(&vq->iotlb_list); >> + >> + snprintf(pool_name, sizeof(pool_name), "iotlb_cache_%d_%d", >> + dev->vid, vq_index); > > iotlb_cache is too generic. Adding a "vhost" prefix? Sure, that would be better. Thanks, Maxime > --yliu >