Re: [dpdk-dev] [PATCH 21/21] vhost: iotlb: reduce iotlb read lock usage

[Yuanhan Liu <yliu@fridaylinux.org>]

On Mon, Sep 11, 2017 at 09:34:30AM +0200, Maxime Coquelin wrote:
> Hi Yuanhan,
> 
> On 09/11/2017 06:18 AM, Yuanhan Liu wrote:
> >On Thu, Aug 31, 2017 at 11:50:23AM +0200, Maxime Coquelin wrote:
> >>Prior to this patch, iotlb cache's read/write lock was
> >>read-locked at every guest IOVA to app VA translation,
> >>i.e. at least once per packet with indirect off and twice
> >>with indirect on.
> >>
> >>The problem is that rte_rwlock_read_lock() makes use of atomic
> >>operation, which is costly.
> >>
> >>This patch introduces iotlb lock helpers, so that a full burst
> >>can be protected with taking the lock once, which reduces the
> >>number of atomic operations by up to 64 with indirect
> >>descriptors.
> >
> >You were assuming there is no single miss during a burst. If a miss
> >happens, it requries 2 locks: one for _pending_miss and another one
> >for _pending_insert. From this point of view, it's actually more
> >expensive.
> 
> It's actually more expensive only when a miss happens.

Yes, that's what I meant.

> And in that case,
> the cost of taking the lock is negligible compared to the miss itself.

Yes, I'm aware of it.

> >However, I won't call it's a bad assumption (for the case of virtio
> >PMD). And if you take this assumption, why not just deleting the
> >pending list and moving the lock outside the _iotlb_find function()
> >like what you did in this patch?
> 
> Because we need the pending list. When the is no matching entry in the
> IOTLB cache, we have to send a miss request through the slave channel.

You don't have the pending list to send a MISS.

> On miss request reception, Qemu performs the translation, and in case of
> success, sends it through the main channel using an update request.
> 
> While all this is done, the backend could wait for it, blocking
> processing on the PMD thread. But it would be really inefficient in case
> other queues are being processed on the same lcore. Moreover, if the
> iova is invalid, no update requst is sent, so the lcore would be blocked
> forever.
> 
> To overcome this, what is done is that in case of miss, it exits the
> burst and try again later, letting a chance for other virtqueues to be
> processed while the update arrives.

You can also quit earlier without the pending list.

> And here comes the pending list. On the next try, the update may have
> not arrived yet, so we need the check whether a miss has already been
> sent for the same address & perm. Else, we would flood Qemu with miss
> requests for the same address.

Okay, that's the reason we need a pending list: to record the miss
we have already sent.

> >I don't really see the point of introducing the pending list.
> 
> Hope the above clarifies.

Thanks, it indeed helps!

> I will see if I can improve the pending list protection, but honestly,
> its cost is negligible.

That's not my point :)

	--yliu