From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f179.google.com (mail-pf0-f179.google.com [209.85.192.179]) by dpdk.org (Postfix) with ESMTP id 8EDC97CBC for ; Fri, 8 Sep 2017 10:09:07 +0200 (CEST) Received: by mail-pf0-f179.google.com with SMTP id e1so3478339pfk.1 for ; Fri, 08 Sep 2017 01:09:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fridaylinux-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=uF4FizYD8xD7+lyav5jmFLP0X8oAep2enireiSvEJ8M=; b=0/Q9OYvBY8qOiG7qIPDKZ2e3Ri7/ukXo4cVfu8ieNyRCLawFNxo2GJxkKOXpFXPIcG 1P/CWnk3Q6x/vv9bK5rEnXa5OvysC1qT8sfgmWv0zPKHF85LnTf59xXoqBqtKLpsJL7P oFNFow06AXKdSTmZhz4W5X52gZerWC1eirEvjX8aemAdctfelfONdGluzJbygMUjCMwS Abj//pGsprzbvqU8dOxT/zvjleCuBoyEB/ST3WZ+lg4HUObcZiAdlXs7egOiblg3N2VS TVKYj/i2ZoAxvvBgJ2WI33whXFVwN7UOj0u5M+VdM3MVQpZa9ig176oiDhCqv3Adc7c2 D9DA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=uF4FizYD8xD7+lyav5jmFLP0X8oAep2enireiSvEJ8M=; b=aO7txqgaAAkYOwng+kMTNOJRtSmdAskV/psXeo26u1T5mSbVAApraslb6Spf5JHl5c 0Uzfn4b/n55J7xbiU+I+/QSWONCeYPFObhmJo9nHY2TL0MN4j7mBbRZA7irFkXMRuS9A 0VcIQFQ8BD/AnXuuXBk9fvxqCpC0F/Dp5QSCLRVIUNnSebswaDU91Hevuf5FTdSA9Az5 dfU1b8IqQ5ydiHHkw7fbb6ItDP2NjYWp/1JPna+xtwTcWC39Y8IBFA+44eIZF5+RUoWv Ek2/gk2aqVyOaGzrtHdUkSZTrO2cEdgYHhxu/mJW32YMOw6GB9ajRwexDfhbzsGoNoyt OTcQ== X-Gm-Message-State: AHPjjUi7KgfomH3VTiitwmiaY3L8HZrzG5Mmjl//GxHFq+w9uwYuAaOu jtNcpk5ANrcV5PWW X-Google-Smtp-Source: ADKCNb4vgaQvzrrQNjp4RngrDhlYtlxz+qxmNgt5HbyvfsnVjnJXuctCifQKUKccS3aJSnM37Q2Zjg== X-Received: by 10.98.26.74 with SMTP id a71mr2265039pfa.286.1504858146707; Fri, 08 Sep 2017 01:09:06 -0700 (PDT) Received: from yliu-home ([45.63.61.64]) by smtp.gmail.com with ESMTPSA id z24sm2702749pfk.3.2017.09.08.01.09.02 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 08 Sep 2017 01:09:05 -0700 (PDT) Date: Fri, 8 Sep 2017 16:08:55 +0800 From: Yuanhan Liu To: Maxime Coquelin Cc: dev@dpdk.org, jfreiman@redhat.com, tiwei.bie@intel.com, mst@redhat.com, vkaplans@redhat.com, jasowang@redhat.com Message-ID: <20170908080855.GF9736@yliu-home> References: <20170831095023.21037-1-maxime.coquelin@redhat.com> <20170831095023.21037-8-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170831095023.21037-8-maxime.coquelin@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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:09:07 -0000 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. [...] > +#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. > +{ > + 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? > + 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. > + 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? --yliu