From: Yuanhan Liu <yliu@fridaylinux.org>
To: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: dev@dpdk.org, jfreiman@redhat.com, tiwei.bie@intel.com,
mst@redhat.com, vkaplans@redhat.com, jasowang@redhat.com
Subject: Re: [dpdk-dev] [PATCH 07/21] vhost: add iotlb helper functions
Date: Fri, 8 Sep 2017 16:08:55 +0800 [thread overview]
Message-ID: <20170908080855.GF9736@yliu-home> (raw)
In-Reply-To: <20170831095023.21037-8-maxime.coquelin@redhat.com>
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 <maxime.coquelin@redhat.com>
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
next prev parent reply other threads:[~2017-09-08 8:09 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-31 9:50 [dpdk-dev] [PATCH 00/21] Vhost-user: Implement device IOTLB support Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 01/21] Revert "vhost: workaround MQ fails to startup" Maxime Coquelin
2017-09-07 11:54 ` Yuanhan Liu
2017-09-07 12:59 ` Maxime Coquelin
2017-09-24 10:41 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 02/21] vhost: make error handling consistent in rx path Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 03/21] vhost: protect virtio_net device struct Maxime Coquelin
2017-09-05 4:45 ` Tiwei Bie
2017-09-05 9:24 ` Maxime Coquelin
2017-09-05 10:07 ` Tiwei Bie
2017-09-05 11:00 ` Maxime Coquelin
2017-09-06 1:15 ` Tiwei Bie
2017-09-06 2:59 ` Stephen Hemminger
2017-09-06 7:50 ` Maxime Coquelin
2017-09-06 7:15 ` Maxime Coquelin
2017-09-06 7:30 ` Tiwei Bie
2017-09-06 20:02 ` Maxime Coquelin
2017-09-07 5:08 ` Tiwei Bie
2017-09-07 13:44 ` Yuanhan Liu
2017-09-07 14:01 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 04/21] vhost: prepare send_vhost_message() to slave requests Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 05/21] vhost: add support to slave requests channel Maxime Coquelin
2017-09-05 4:19 ` Tiwei Bie
2017-09-05 8:18 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 06/21] vhost: declare missing IOMMU-related definitions for old kernels Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 07/21] vhost: add iotlb helper functions Maxime Coquelin
2017-09-05 6:02 ` Tiwei Bie
2017-09-05 15:16 ` Maxime Coquelin
2017-09-08 8:08 ` Yuanhan Liu [this message]
2017-09-08 8:24 ` Maxime Coquelin
2017-09-08 8:36 ` Yuanhan Liu
2017-09-08 8:50 ` Maxime Coquelin
2017-09-08 9:21 ` Yuanhan Liu
2017-09-08 9:28 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 08/21] vhost: iotlb: add pending miss request list and helpers Maxime Coquelin
2017-09-05 7:11 ` Tiwei Bie
2017-09-05 15:18 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 09/21] vhost-user: add support to IOTLB miss slave requests Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 10/21] vhost: initialize vrings IOTLB caches Maxime Coquelin
2017-09-04 13:57 ` Remy Horton
2017-09-04 15:45 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 11/21] vhost-user: handle IOTLB update and invalidate requests Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 12/21] vhost: introduce guest IOVA to backend VA helper Maxime Coquelin
2017-09-05 4:14 ` Tiwei Bie
2017-09-05 7:05 ` Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 13/21] vhost: use the guest IOVA to host " Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 14/21] vhost: enable rings at the right time Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 15/21] vhost: don't dereference invalid dev pointer after its reallocation Maxime Coquelin
2017-09-04 13:58 ` Remy Horton
2017-08-31 9:50 ` [dpdk-dev] [PATCH 16/21] vhost: postpone rings addresses translation Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 17/21] vhost-user: translate ring addresses when IOMMU enabled Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 18/21] vhost-user: iommu: postpone device creation until ring are mapped Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 19/21] vhost: iommu: Invalidate vring in case of matching IOTLB invalidate Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 20/21] vhost: enable IOMMU support Maxime Coquelin
2017-08-31 9:50 ` [dpdk-dev] [PATCH 21/21] vhost: iotlb: reduce iotlb read lock usage Maxime Coquelin
2017-09-11 4:18 ` Yuanhan Liu
2017-09-11 7:34 ` Maxime Coquelin
2017-09-11 9:39 ` Yuanhan Liu
2017-09-04 13:58 ` [dpdk-dev] [PATCH 00/21] Vhost-user: Implement device IOTLB support Remy Horton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170908080855.GF9736@yliu-home \
--to=yliu@fridaylinux.org \
--cc=dev@dpdk.org \
--cc=jasowang@redhat.com \
--cc=jfreiman@redhat.com \
--cc=maxime.coquelin@redhat.com \
--cc=mst@redhat.com \
--cc=tiwei.bie@intel.com \
--cc=vkaplans@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).