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