On Wed, 31 Mar 2021 15:47:45 +0100 Stefan Hajnoczi <stefa...@redhat.com> wrote:
> On Tue, Mar 30, 2021 at 04:17:32PM +0200, Greg Kurz wrote:
> > On Tue, 30 Mar 2021 14:55:42 +0100
> > Stefan Hajnoczi <stefa...@redhat.com> wrote:
> >
> > > On Tue, Mar 30, 2021 at 12:17:40PM +0200, Greg Kurz wrote:
> > > > On Mon, 29 Mar 2021 18:10:57 +0100
> > > > Stefan Hajnoczi <stefa...@redhat.com> wrote:
> > > > > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote:
> > > > > > @@ -315,6 +338,10 @@ static void
> > > > > > virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus,
> > > > > >
> > > > > > for (i = 0; i < nvqs; i++) {
> > > > > > virtio_bus_set_host_notifier(bus, i + n_offset, false);
> > > > > > + }
> > > > > > + /* Let address_space_update_ioeventfds() run before closing
> > > > > > ioeventfds */
> > > > >
> > > > > assert(memory_region_transaction_depth == 0)?
> > > > >
> > > >
> > > > Hmm... appart from the fact that memory_region_transaction_depth is
> > > > a memory internal thing that shouldn't be exposed here, it seems to
> > > > me that memory_region_transaction_depth can be != 0 when, e.g. when
> > > > batching is used... or I'm missing something ?
> > > >
> > > > I was actually thinking of adding some asserts for that in the
> > > > memory_region_*_eventfd_full() functions introduced by patch 1.
> > > >
> > > > if (!transaction) {
> > > > memory_region_transaction_begin();
> > > > }
> > > > assert(memory_region_transaction_depth != 0);
> > >
> > > In that case is it safe to call virtio_bus_cleanup_host_notifier()
> > > below? I thought it depends on the transaction committing first.
> > >
> >
> > Yes because the transaction ends...
> >
> > > >
> > > > > > + virtio_bus_set_host_notifier_commit(bus);
> > ... here ^^
> >
> > > > > > + for (i = 0; i < nvqs; i++) {
> > > > > > virtio_bus_cleanup_host_notifier(bus, i + n_offset);
> > > > > > }
> > > > > > }
>
> That contradicts what you said above: "it seems to me that
> memory_region_transaction_depth can be != 0 when, e.g. when batching is
> used".
>
> If memory_region_transaction_depth can be != 0 when this function is
> entered then memory_region_transaction_commit() will have no effect:
>
> void memory_region_transaction_commit(void)
> {
> AddressSpace *as;
>
> assert(memory_region_transaction_depth);
> assert(qemu_mutex_iothread_locked());
>
> --memory_region_transaction_depth;
> if (!memory_region_transaction_depth) {
memory_region_transaction_depth should be equal to 1 when
entering the function, not 0... which is the case when
batching.
> ^--- we won't take this branch!
>
> So the code after memory_region_transaction_commit() cannot assume that
> anything was actually committed.
>
Right and nothing in the current code base seems to prevent
memory_region_*_eventfd() to be called within an ongoing
transaction actually. It looks that we might want to fix that
first.
> That's why I asked about adding assert(memory_region_transaction_depth
> == 0) to guarantee that our commit takes effect immediately so that it's
> safe to call virtio_bus_cleanup_host_notifier().
>
Yes, it was just misplaced and I didn't get the intent at first :)
> Stefan
pgpw1Ocu_g85n.pgp
Description: OpenPGP digital signature
