Re: acceptance-system-fedora failures

[Philippe Mathieu-Daudé]

On 10/7/20 2:20 PM, Pavel Dovgalyuk wrote:
> On 07.10.2020 14:22, Alex Bennée wrote:
>>
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 10/7/20 10:51 AM, Pavel Dovgalyuk wrote:
>>>> On 07.10.2020 11:23, Thomas Huth wrote:
>>>>> On 07/10/2020 09.13, Philippe Mathieu-Daudé wrote:
>>>>> Thanks, that was helpful. ... and the winner is:
>>>>>
>>>>>       commit   55adb3c45620c31f29978f209e2a44a08d34e2da
>>>>>       Author:  John Snow <jsnow@redhat.com>
>>>>>       Date:    Fri Jul 24 01:23:00 2020 -0400
>>>>>       Subject: ide: cancel pending callbacks on SRST
>>>>>
>>>>> ... starting with this commit, the tests starts failing. John, any
>>>>> idea what
>>>>> might be causing this?
>>>>
>>>> This patch includes the following lines:
>>>>
>>>> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
>>>> +                                ide_bus_perform_srst, bus);
>>>>
>>>> replay_bh_schedule_oneshot_event should be used instead of this
>>>> function, because it synchronizes non-deterministic BHs.
>>>
>>> Why do we have 2 different functions? BH are already complex
>>> enough, and we need to also think about the replay API...
>>>
>>> What about the other cases such vhost-user (blk/net), virtio-blk?
>>
>> This does seem like something that should be wrapped up inside
>> aio_bh_schedule_oneshot itself or maybe we need a
>> aio_bh_schedule_transaction_oneshot to distinguish it from the other
>> uses the function has.
>>
> 
> Maybe there should be two functions:
> - one for the guest modification

aio_bh_schedule_oneshot_deterministic()?

> - one for internal qemu things

Not sure why there is a difference, BH are used to
avoid delaying the guest, so there always something
related to "guest modification".

> 
> The first one may be implemented though the rr+second one.
> Maybe replay_ prefix is confusing and the whole BH interface should look
> like that?

Yes, but it would be safer/clearer if we don't need to use
a replay_ API.

Can we embed these functions?

- replay_bh_schedule_event
- replay_bh_schedule_oneshot_event

If compiled without rr, events_enabled=false and
compiler can optimize:

-- >8 --
diff --git a/util/async.c b/util/async.c
index f758354c6a..376b6d4e27 100644
--- a/util/async.c
+++ b/util/async.c
@@ -109,14 +109,19 @@ static QEMUBH *aio_bh_dequeue(BHList *head,
unsigned *flags)

 void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
 {
-    QEMUBH *bh;
-    bh = g_new(QEMUBH, 1);
-    *bh = (QEMUBH){
-        .ctx = ctx,
-        .cb = cb,
-        .opaque = opaque,
-    };
-    aio_bh_enqueue(bh, BH_SCHEDULED | BH_ONESHOT);
+    if (events_enabled) {
+        replay_add_event(REPLAY_ASYNC_EVENT_BH_ONESHOT, cb,
+                         opaque, replay_get_current_icount());
+    } else {
+        QEMUBH *bh;
+        bh = g_new(QEMUBH, 1);
+        *bh = (QEMUBH){
+            .ctx = ctx,
+            .cb = cb,
+            .opaque = opaque,
+        };
+        aio_bh_enqueue(bh, BH_SCHEDULED | BH_ONESHOT);
+    }
 }

 QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
@@ -178,7 +183,12 @@ void qemu_bh_schedule_idle(QEMUBH *bh)

 void qemu_bh_schedule(QEMUBH *bh)
 {
-    aio_bh_enqueue(bh, BH_SCHEDULED);
+    if (events_enabled) {
+        replay_add_event(REPLAY_ASYNC_EVENT_BH, bh, NULL,
+                         replay_get_current_icount());
+    } else {
+        aio_bh_enqueue(bh, BH_SCHEDULED);
+    }
 }

---

> 
> 
> Pavel Dovgalyuk
>