On Wed, Nov 19, 2025 at 10:32 AM Michael S. Tsirkin <[email protected]> wrote: > > On Wed, Nov 19, 2025 at 10:26:43AM +0100, Eugenio Perez Martin wrote: > > > But this is not the logic that is > > > implemented in this patch as there's no synchronize_rcu() in the > > > vduse_set_group_asid_nomsg(). > > > > We only set the pointer on the writer's side, we do nothing like > > freeing resources. Should we set the pointer before or after > > syncrhonize_rcu()? > > synchronize_rcu is called after writer makes it's changes. > > > What do we need to do on the other side of > > syncrhonize_rcu()? > > Presumably, return so the caller knows the as has been updated. >
I'm happy to add the syncrhonize_rcu() just in case, but the caller of vduse_set_group_asid_nomsg does not need to know that the reader has been updated. The first caller is vduse_dev_reset which has its own way to avoid being called while vqs are being processed. In particular, it reset their addresses which is way more dangerous. I could call it with dev->rwsem down though. The second one is set_group_asid vdpa callback which is called from the ioctl itself. Moreover, rcu_assign_pointer is WRITE_ONCE by itself so we know all the readers will get the new value after it. So what's the value of explicitly waiting for all the readers to finalize their DMA operation? I'd understand if we need to do modifications or memory management in the now unused ASID, but that's not the case here. > However, user-triggerable synchronize_rcu() is almost always a bug. > > If that's what is going on, you want srcu. > I did not know about it, thanks! but I think all the code that can sleep is out of the RCU critical sections now, isn't it?
