Re: acceptance-system-fedora failures

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/7/20 3:11 PM, Pavel Dovgalyuk wrote:
> On 07.10.2020 15:49, Philippe Mathieu-Daudé wrote:
>> 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".
> 
> Not exactly. At least there is one non-related-to-guest case
> in monitor_init_qmp:
>         /*
>          * We can't call qemu_chr_fe_set_handlers() directly here
>          * since chardev might be running in the monitor I/O
>          * thread.  Schedule a bottom half.
>          */
>         aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
>                                 monitor_qmp_setup_handlers_bh, mon);

I don't understand the documentation in docs/devel/replay.txt:

---
Bottom halves
=============

Bottom half callbacks, that affect the guest state, should be invoked
through
replay_bh_schedule_event or replay_bh_schedule_oneshot_event functions.
Their invocations are saved in record mode and synchronized with the
existing
log in replay mode.
---

But then it is only used in block drivers, which are not
related to guest state:

$ git grep replay_bh_schedule_oneshot_event
block/block-backend.c:1385:
replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
block/block-backend.c:1450:
replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
block/io.c:371:
replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
block/iscsi.c:286:
replay_bh_schedule_oneshot_event(iTask->iscsilun->aio_context,
block/nfs.c:262:
replay_bh_schedule_oneshot_event(task->client->aio_context,
block/null.c:183:
replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
block/nvme.c:323:        replay_bh_schedule_oneshot_event(q->aio_context,
block/nvme.c:1075:    replay_bh_schedule_oneshot_event(data->ctx,
nvme_rw_cb_bh, data);
block/rbd.c:865:
replay_bh_schedule_oneshot_event(bdrv_get_aio_context(acb->common.bs),
docs/devel/replay.txt:25:replay_bh_schedule_event or
replay_bh_schedule_oneshot_event functions.
include/sysemu/replay.h:178:void
replay_bh_schedule_oneshot_event(AioContext *ctx,
replay/replay-events.c:141:void
replay_bh_schedule_oneshot_event(AioContext *ctx,
stubs/replay-user.c:5:void replay_bh_schedule_oneshot_event(AioContext *ctx,

Is replay_bh_schedule_oneshot_event ever used by replay?
Maybe we can remove it and use aio_bh_schedule_oneshot()
in place?

Else the documentation need to be clarified please.

> 
> 
>>
>>>
>>> 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)
> 
> qemu_bh_schedule is even worse.
> Many modules use it (e.g., loadvm_postcopy_handle_run), and there is no
> need to use replay version there. In such cases QEMU will halt if trying
> to call replay_bh_schedule_event.
> 
>>   {
>> -    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);
>> +    }
>>   }
>>
>