Re: [dpdk-dev] [PATCH 07/21] vhost: add iotlb helper functions

[Yuanhan Liu <yliu@fridaylinux.org>]

On Fri, Sep 08, 2017 at 10:50:49AM +0200, Maxime Coquelin wrote:
> >>>>+{
> >>>>+	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.
> >
> >You were removing all caches, how can we do worse than that? Even a
> >random evict would be better. Or, more simply, just to remove the
> >head or the tail?
> 
> I think removing head or tail could cause deadlocks.
> For example it needs to translate from 0x0 to 0x2000, with page size
> being 0x1000.
> 
> If cache is full, when inserting 0x1000-0x1fff, it will remove
> 0x0-0xfff, so a miss will be sent for 0x0-0xffff and 0x1000-0x1fff will
> be remove at insert time, etc...

Okay, that means we can't simply remove the head or tail.

> Note that we really need to size the cache large enough for performance
> purpose, so having the cache full could be cause by either buggy or
> malicious guest.

I agree. But for the malicious guest, it could lead to a DDOS attack:
assume it keeps vhost running out of cache and then vhost keeps printing
above message.

What I suggested was to evict one (by some polices) to get a space for
the new one we want to insert. Note that it won't be a performance issue,
IMO, due to we only do that when we run out of caches, which, according
to your sayings, should not happen in normal cases.

	--yliu

> >>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;
> >>>>+		}
> >>>>+	}