| Message ID | 20240116143622.6641-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Drop out of coroutine context xen_invalidate_map_cache_entry | expand |
On Tue, 16 Jan 2024, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > xen_invalidate_map_cache_entry is not expected to run in a > coroutine. Without this, there is crash: > > signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 > threadid=<optimized out>) at pthread_kill.c:78 > at /usr/src/debug/glibc/2.38+git-r0/sysdeps/posix/raise.c:26 > fmt=0xffff9e1ca8a8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", > assertion=assertion@entry=0xaaaae0d25740 "!qemu_in_coroutine()", > file=file@entry=0xaaaae0d301a8 "../qemu-xen-dir-remote/block/graph-lock.c", line=line@entry=260, > function=function@entry=0xaaaae0e522c0 <__PRETTY_FUNCTION__.3> "bdrv_graph_rdlock_main_loop") at assert.c:92 > assertion=assertion@entry=0xaaaae0d25740 "!qemu_in_coroutine()", > file=file@entry=0xaaaae0d301a8 "../qemu-xen-dir-remote/block/graph-lock.c", line=line@entry=260, > function=function@entry=0xaaaae0e522c0 <__PRETTY_FUNCTION__.3> "bdrv_graph_rdlock_main_loop") at assert.c:101 > at ../qemu-xen-dir-remote/block/graph-lock.c:260 > at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir-remote/include/block/graph-lock.h:259 > host=host@entry=0xffff742c8000, size=size@entry=2097152) > at ../qemu-xen-dir-remote/block/io.c:3362 > host=0xffff742c8000, size=2097152) > at ../qemu-xen-dir-remote/block/block-backend.c:2859 > host=<optimized out>, size=<optimized out>, max_size=<optimized out>) > at ../qemu-xen-dir-remote/block/block-ram-registrar.c:33 > size=2097152, max_size=2097152) > at ../qemu-xen-dir-remote/hw/core/numa.c:883 > buffer=buffer@entry=0xffff743c5000 "") > at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:475 > buffer=buffer@entry=0xffff743c5000 "") > at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:487 > as=as@entry=0xaaaae1ca3ae8 <address_space_memory>, buffer=0xffff743c5000, > len=<optimized out>, is_write=is_write@entry=true, > access_len=access_len@entry=32768) > at ../qemu-xen-dir-remote/system/physmem.c:3199 > dir=DMA_DIRECTION_FROM_DEVICE, len=<optimized out>, > buffer=<optimized out>, as=0xaaaae1ca3ae8 <address_space_memory>) > at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir-remote/include/sysemu/dma.h:236 > elem=elem@entry=0xaaaaf620aa30, len=len@entry=32769) > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:758 > elem=elem@entry=0xaaaaf620aa30, len=len@entry=32769, idx=idx@entry=0) > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:919 > elem=elem@entry=0xaaaaf620aa30, len=32769) > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:994 > req=req@entry=0xaaaaf620aa30, status=status@entry=0 '\000') > at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:67 > ret=0) at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:136 > at ../qemu-xen-dir-remote/block/block-backend.c:1559 > --Type <RET> for more, q to quit, c to continue without paging-- > at ../qemu-xen-dir-remote/block/block-backend.c:1614 > i1=<optimized out>) at ../qemu-xen-dir-remote/util/coroutine-ucontext.c:177 > at ../sysdeps/unix/sysv/linux/aarch64/setcontext.S:123 > > Signed-off-by: Peng Fan <peng.fan@nxp.com> Hi Peng! Many thanks for the patch and for the investigation! Only one minor question below > --- > hw/xen/xen-mapcache.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c > index f7d974677d..4e1bb665ee 100644 > --- a/hw/xen/xen-mapcache.c > +++ b/hw/xen/xen-mapcache.c > @@ -481,11 +481,38 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer) > g_free(entry); > } > > -void xen_invalidate_map_cache_entry(uint8_t *buffer) > +typedef struct XenMapCacheData { > + Coroutine *co; > + uint8_t *buffer; > + int ret; Do we need int ret? It doesn't look like we are using it. > +} XenMapCacheData; > + > +static void xen_invalidate_map_cache_entry_bh(void *opaque) > { > + XenMapCacheData *data = opaque; > + > mapcache_lock(); > - xen_invalidate_map_cache_entry_unlocked(buffer); > + xen_invalidate_map_cache_entry_unlocked(data->buffer); > mapcache_unlock(); > + > + aio_co_wake(data->co); > +} > + > +void coroutine_mixed_fn xen_invalidate_map_cache_entry(uint8_t *buffer) > +{ > + if (qemu_in_coroutine()) { > + XenMapCacheData data = { > + .co = qemu_coroutine_self(), > + .buffer = buffer, > + }; > + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), > + xen_invalidate_map_cache_entry_bh, &data); > + qemu_coroutine_yield(); > + } else { > + mapcache_lock(); > + xen_invalidate_map_cache_entry_unlocked(buffer); > + mapcache_unlock(); > + } > } > > void xen_invalidate_map_cache(void) > -- > 2.35.3 >
> Subject: Re: [PATCH] xen: Drop out of coroutine context > xen_invalidate_map_cache_entry > > On Tue, 16 Jan 2024, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > xen_invalidate_map_cache_entry is not expected to run in a coroutine. > > Without this, there is crash: > > > > signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 > > threadid=<optimized out>) at pthread_kill.c:78 > > at /usr/src/debug/glibc/2.38+git-r0/sysdeps/posix/raise.c:26 > > fmt=0xffff9e1ca8a8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", > > assertion=assertion@entry=0xaaaae0d25740 "!qemu_in_coroutine()", > > file=file@entry=0xaaaae0d301a8 "../qemu-xen-dir-remote/block/graph- > lock.c", line=line@entry=260, > > function=function@entry=0xaaaae0e522c0 <__PRETTY_FUNCTION__.3> > "bdrv_graph_rdlock_main_loop") at assert.c:92 > > assertion=assertion@entry=0xaaaae0d25740 "!qemu_in_coroutine()", > > file=file@entry=0xaaaae0d301a8 "../qemu-xen-dir-remote/block/graph- > lock.c", line=line@entry=260, > > function=function@entry=0xaaaae0e522c0 <__PRETTY_FUNCTION__.3> > "bdrv_graph_rdlock_main_loop") at assert.c:101 > > at ../qemu-xen-dir-remote/block/graph-lock.c:260 > > at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir- > remote/include/block/graph-lock.h:259 > > host=host@entry=0xffff742c8000, size=size@entry=2097152) > > at ../qemu-xen-dir-remote/block/io.c:3362 > > host=0xffff742c8000, size=2097152) > > at ../qemu-xen-dir-remote/block/block-backend.c:2859 > > host=<optimized out>, size=<optimized out>, max_size=<optimized out>) > > at ../qemu-xen-dir-remote/block/block-ram-registrar.c:33 > > size=2097152, max_size=2097152) > > at ../qemu-xen-dir-remote/hw/core/numa.c:883 > > buffer=buffer@entry=0xffff743c5000 "") > > at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:475 > > buffer=buffer@entry=0xffff743c5000 "") > > at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:487 > > as=as@entry=0xaaaae1ca3ae8 <address_space_memory>, > buffer=0xffff743c5000, > > len=<optimized out>, is_write=is_write@entry=true, > > access_len=access_len@entry=32768) > > at ../qemu-xen-dir-remote/system/physmem.c:3199 > > dir=DMA_DIRECTION_FROM_DEVICE, len=<optimized out>, > > buffer=<optimized out>, as=0xaaaae1ca3ae8 <address_space_memory>) > > at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir- > remote/include/sysemu/dma.h:236 > > elem=elem@entry=0xaaaaf620aa30, len=len@entry=32769) > > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:758 > > elem=elem@entry=0xaaaaf620aa30, len=len@entry=32769, > idx=idx@entry=0) > > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:919 > > elem=elem@entry=0xaaaaf620aa30, len=32769) > > at ../qemu-xen-dir-remote/hw/virtio/virtio.c:994 > > req=req@entry=0xaaaaf620aa30, status=status@entry=0 '\000') > > at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:67 > > ret=0) at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:136 > > at ../qemu-xen-dir-remote/block/block-backend.c:1559 > > --Type <RET> for more, q to quit, c to continue without paging-- > > at ../qemu-xen-dir-remote/block/block-backend.c:1614 > > i1=<optimized out>) at ../qemu-xen-dir-remote/util/coroutine- > ucontext.c:177 > > at ../sysdeps/unix/sysv/linux/aarch64/setcontext.S:123 > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > Hi Peng! Many thanks for the patch and for the investigation! > > Only one minor question below > > > > --- > > hw/xen/xen-mapcache.c | 31 +++++++++++++++++++++++++++++-- > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c index > > f7d974677d..4e1bb665ee 100644 > > --- a/hw/xen/xen-mapcache.c > > +++ b/hw/xen/xen-mapcache.c > > @@ -481,11 +481,38 @@ static void > xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer) > > g_free(entry); > > } > > > > -void xen_invalidate_map_cache_entry(uint8_t *buffer) > > +typedef struct XenMapCacheData { > > + Coroutine *co; > > + uint8_t *buffer; > > + int ret; > > Do we need int ret? It doesn't look like we are using it. Good catch, it is not needed, I will drop it in V2. Thanks, Peng. > > > > +} XenMapCacheData; > > + > > +static void xen_invalidate_map_cache_entry_bh(void *opaque) > > { > > + XenMapCacheData *data = opaque; > > + > > mapcache_lock(); > > - xen_invalidate_map_cache_entry_unlocked(buffer); > > + xen_invalidate_map_cache_entry_unlocked(data->buffer); > > mapcache_unlock(); > > + > > + aio_co_wake(data->co); > > +} > > + > > +void coroutine_mixed_fn xen_invalidate_map_cache_entry(uint8_t > > +*buffer) { > > + if (qemu_in_coroutine()) { > > + XenMapCacheData data = { > > + .co = qemu_coroutine_self(), > > + .buffer = buffer, > > + }; > > + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), > > + xen_invalidate_map_cache_entry_bh, &data); > > + qemu_coroutine_yield(); > > + } else { > > + mapcache_lock(); > > + xen_invalidate_map_cache_entry_unlocked(buffer); > > + mapcache_unlock(); > > + } > > } > > > > void xen_invalidate_map_cache(void) > > -- > > 2.35.3 > >
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c index f7d974677d..4e1bb665ee 100644 --- a/hw/xen/xen-mapcache.c +++ b/hw/xen/xen-mapcache.c @@ -481,11 +481,38 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer) g_free(entry); } -void xen_invalidate_map_cache_entry(uint8_t *buffer) +typedef struct XenMapCacheData { + Coroutine *co; + uint8_t *buffer; + int ret; +} XenMapCacheData; + +static void xen_invalidate_map_cache_entry_bh(void *opaque) { + XenMapCacheData *data = opaque; + mapcache_lock(); - xen_invalidate_map_cache_entry_unlocked(buffer); + xen_invalidate_map_cache_entry_unlocked(data->buffer); mapcache_unlock(); + + aio_co_wake(data->co); +} + +void coroutine_mixed_fn xen_invalidate_map_cache_entry(uint8_t *buffer) +{ + if (qemu_in_coroutine()) { + XenMapCacheData data = { + .co = qemu_coroutine_self(), + .buffer = buffer, + }; + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), + xen_invalidate_map_cache_entry_bh, &data); + qemu_coroutine_yield(); + } else { + mapcache_lock(); + xen_invalidate_map_cache_entry_unlocked(buffer); + mapcache_unlock(); + } } void xen_invalidate_map_cache(void)