On Tue, Jul 23, 2019 at 04:49:01PM +0800, Jason Wang wrote:

On 2019/7/23 下午4:10, Michael S. Tsirkin wrote:

quoted

On Tue, Jul 23, 2019 at 03:53:06PM +0800, Jason Wang wrote:

quoted

On 2019/7/23 下午3:23, Michael S. Tsirkin wrote:

quoted

quoted

quoted

Really let's just use kfree_rcu. It's way cleaner: fire and forget.

Looks not, you need rate limit the fire as you've figured out?

See the discussion that followed. Basically no, it's good enough
already and is only going to be better.

quoted

And in fact,
the synchronization is not even needed, does it help if I leave a comment to
explain?

Let's try to figure it out in the mail first. I'm pretty sure the
current logic is wrong.

Here is what the code what to achieve:

- The map was protected by RCU

- Writers are: MMU notifier invalidation callbacks, file operations (ioctls
etc), meta_prefetch (datapath)

- Readers are: memory accessor

Writer are synchronized through mmu_lock. RCU is used to synchronized
between writers and readers.

The synchronize_rcu() in vhost_reset_vq_maps() was used to synchronized it
with readers (memory accessors) in the path of file operations. But in this
case, vq->mutex was already held, this means it has been serialized with
memory accessor. That's why I think it could be removed safely.

Anything I miss here?

So invalidate callbacks need to reset the map, and they do
not have vq mutex. How can they do this and free
the map safely? They need synchronize_rcu or kfree_rcu right?

Invalidation callbacks need but file operations (e.g ioctl) not.

quoted

And I worry somewhat that synchronize_rcu in an MMU notifier
is a problem, MMU notifiers are supposed to be quick:

Looks not, since it can allow to be blocked and lots of driver depends on
this. (E.g mmu_notifier_range_blockable()).

Right, they can block. So why don't we take a VQ mutex and be
done with it then? No RCU tricks.

quoted

they are on a read side critical section of SRCU.

If we could get rid of RCU that would be even better.

But now I wonder:
	invalidate_start has to mark page as dirty
	(this is what my patch added, current code misses this).

Nope, current code did this but not the case when map need to be invalidated
in the vhost control path (ioctl etc).

quoted

	at that point kernel can come and make the page clean again.

	At that point VQ handlers can keep a copy of the map
	and change the page again.

We will increase invalidate_count which prevent the page being used by map.

Thanks

OK I think I got it, thanks!

quoted

At this point I don't understand how we can mark page dirty
safely.

quoted

quoted

quoted

quoted

quoted

Btw, for kvm ioctl it still uses synchronize_rcu() in kvm_vcpu_ioctl(),
(just a little bit more hard to trigger):

AFAIK these never run in response to guest events.
So they can take very long and guests still won't crash.

What if guest manages to escape to qemu?

Thanks

Then it's going to be slow. Why do we care?
What we do not want is synchronize_rcu that guest is blocked on.

Ok, this looks like that I have some misunderstanding here of the reason why
synchronize_rcu() is not preferable in the path of ioctl. But in kvm case,
if rcu_expedited is set, it can triggers IPIs AFAIK.

Thanks

Yes, expedited is not good for something guest can trigger.
Let's just use kfree_rcu if we can. Paul said even though
documentation still says it needs to be rate-limited, that
part is basically stale and will get updated.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel