1. Patchew
  2. QEMU
  3. View series

[PATCH] xen: Drop out of coroutine context xen_invalidate_map_cache_entry

Peng Fan (OSS) posted 1 patch 10 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240116143622.6641-1-peng.fan@oss.nxp.com
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>
There is a newer version of this series
hw/xen/xen-mapcache.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
[PATCH] xen: Drop out of coroutine context xen_invalidate_map_cache_entry
Posted by Peng Fan (OSS) 10 months, 2 weeks ago 
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>
---
 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;
+} 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
Re: [PATCH] xen: Drop out of coroutine context xen_invalidate_map_cache_entry
Posted by Stefano Stabellini 10 months, 1 week ago 
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
>
RE: [PATCH] xen: Drop out of coroutine context xen_invalidate_map_cache_entry
Posted by Peng Fan 10 months, 1 week ago 
> 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
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.