diff mbox series

[v3,4/9] replay: simplify async event processing

Message ID 165355472374.533615.18197241124455918315.stgit@pasha-ThinkPad-X280 (mailing list archive)
State New, archived
Headers show
Series Record/replay refactoring and stuff | expand

Commit Message

Pavel Dovgalyuk May 26, 2022, 8:45 a.m. UTC
This patch joins replay event id and async event id into single byte in the log.
It makes processing a bit faster and log a bit smaller.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

--

v2: minor enum fixes (suggested by Richard Henderson)
---
 replay/replay-events.c   |   36 ++++++++++++++----------------------
 replay/replay-internal.h |   31 ++++++++++++++-----------------
 replay/replay-snapshot.c |    1 -
 replay/replay.c          |    5 +++--
 4 files changed, 31 insertions(+), 42 deletions(-)

Comments

Paolo Bonzini May 26, 2022, 9:40 a.m. UTC | #1
On 5/26/22 10:45, Pavel Dovgalyuk wrote:
> +
> +typedef enum ReplayAsyncEventKind {
> +    REPLAY_ASYNC_EVENT_BH,
> +    REPLAY_ASYNC_EVENT_BH_ONESHOT,
> +    REPLAY_ASYNC_EVENT_INPUT,
> +    REPLAY_ASYNC_EVENT_INPUT_SYNC,
> +    REPLAY_ASYNC_EVENT_CHAR_READ,
> +    REPLAY_ASYNC_EVENT_BLOCK,
> +    REPLAY_ASYNC_EVENT_NET,
> +    REPLAY_ASYNC_COUNT
> +} ReplayAsyncEventKind;
> +
>   /* Any changes to order/number of events will need to bump REPLAY_VERSION */
>   enum ReplayEvents {
>       /* for instruction event */
> @@ -22,6 +35,7 @@ enum ReplayEvents {
>       EVENT_EXCEPTION,
>       /* for async events */
>       EVENT_ASYNC,
> +    EVENT_ASYNC_LAST = EVENT_ASYNC + REPLAY_ASYNC_COUNT - 1,

Why not unify the two enums into one?

Paolo
Pavel Dovgalyuk May 26, 2022, 9:53 a.m. UTC | #2
On 26.05.2022 12:40, Paolo Bonzini wrote:
> On 5/26/22 10:45, Pavel Dovgalyuk wrote:
>> +
>> +typedef enum ReplayAsyncEventKind {
>> +    REPLAY_ASYNC_EVENT_BH,
>> +    REPLAY_ASYNC_EVENT_BH_ONESHOT,
>> +    REPLAY_ASYNC_EVENT_INPUT,
>> +    REPLAY_ASYNC_EVENT_INPUT_SYNC,
>> +    REPLAY_ASYNC_EVENT_CHAR_READ,
>> +    REPLAY_ASYNC_EVENT_BLOCK,
>> +    REPLAY_ASYNC_EVENT_NET,
>> +    REPLAY_ASYNC_COUNT
>> +} ReplayAsyncEventKind;
>> +
>>   /* Any changes to order/number of events will need to bump 
>> REPLAY_VERSION */
>>   enum ReplayEvents {
>>       /* for instruction event */
>> @@ -22,6 +35,7 @@ enum ReplayEvents {
>>       EVENT_EXCEPTION,
>>       /* for async events */
>>       EVENT_ASYNC,
>> +    EVENT_ASYNC_LAST = EVENT_ASYNC + REPLAY_ASYNC_COUNT - 1,
> 
> Why not unify the two enums into one?

ReplayAsyncEventKind is used as a variable type.
I think narrow type describes the value better than common enum.

> 
> Paolo
diff mbox series

Patch

diff --git a/replay/replay-events.c b/replay/replay-events.c
index db1decf9dd..af0721cc1a 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -174,8 +174,8 @@  static void replay_save_event(Event *event)
 {
     if (replay_mode != REPLAY_MODE_PLAY) {
         /* put the event into the file */
-        replay_put_event(EVENT_ASYNC);
-        replay_put_byte(event->event_kind);
+        g_assert(event->event_kind < REPLAY_ASYNC_COUNT);
+        replay_put_event(EVENT_ASYNC + event->event_kind);
 
         /* save event-specific data */
         switch (event->event_kind) {
@@ -220,14 +220,10 @@  void replay_save_events(void)
 static Event *replay_read_event(void)
 {
     Event *event;
-    if (replay_state.read_event_kind == -1) {
-        replay_state.read_event_kind = replay_get_byte();
-        replay_state.read_event_id = -1;
-        replay_check_error();
-    }
+    ReplayAsyncEventKind event_kind = replay_state.data_kind - EVENT_ASYNC;
 
     /* Events that has not to be in the queue */
-    switch (replay_state.read_event_kind) {
+    switch (event_kind) {
     case REPLAY_ASYNC_EVENT_BH:
     case REPLAY_ASYNC_EVENT_BH_ONESHOT:
         if (replay_state.read_event_id == -1) {
@@ -236,17 +232,17 @@  static Event *replay_read_event(void)
         break;
     case REPLAY_ASYNC_EVENT_INPUT:
         event = g_new0(Event, 1);
-        event->event_kind = replay_state.read_event_kind;
+        event->event_kind = event_kind;
         event->opaque = replay_read_input_event();
         return event;
     case REPLAY_ASYNC_EVENT_INPUT_SYNC:
         event = g_new0(Event, 1);
-        event->event_kind = replay_state.read_event_kind;
+        event->event_kind = event_kind;
         event->opaque = 0;
         return event;
     case REPLAY_ASYNC_EVENT_CHAR_READ:
         event = g_new0(Event, 1);
-        event->event_kind = replay_state.read_event_kind;
+        event->event_kind = event_kind;
         event->opaque = replay_event_char_read_load();
         return event;
     case REPLAY_ASYNC_EVENT_BLOCK:
@@ -256,18 +252,17 @@  static Event *replay_read_event(void)
         break;
     case REPLAY_ASYNC_EVENT_NET:
         event = g_new0(Event, 1);
-        event->event_kind = replay_state.read_event_kind;
+        event->event_kind = event_kind;
         event->opaque = replay_event_net_load();
         return event;
     default:
-        error_report("Unknown ID %d of replay event",
-            replay_state.read_event_kind);
+        error_report("Unknown ID %d of replay event", event_kind);
         exit(1);
         break;
     }
 
     QTAILQ_FOREACH(event, &events_list, events) {
-        if (event->event_kind == replay_state.read_event_kind
+        if (event->event_kind == event_kind
             && (replay_state.read_event_id == -1
                 || replay_state.read_event_id == event->id)) {
             break;
@@ -276,12 +271,8 @@  static Event *replay_read_event(void)
 
     if (event) {
         QTAILQ_REMOVE(&events_list, event, events);
-    } else {
-        return NULL;
     }
 
-    /* Read event-specific data */
-
     return event;
 }
 
@@ -289,13 +280,14 @@  static Event *replay_read_event(void)
 void replay_read_events(void)
 {
     g_assert(replay_mutex_locked());
-    while (replay_state.data_kind == EVENT_ASYNC) {
+    while (replay_state.data_kind >= EVENT_ASYNC
+        && replay_state.data_kind <= EVENT_ASYNC_LAST) {
         Event *event = replay_read_event();
         if (!event) {
             break;
         }
         replay_finish_event();
-        replay_state.read_event_kind = -1;
+        replay_state.read_event_id = -1;
         replay_run_event(event);
 
         g_free(event);
@@ -304,7 +296,7 @@  void replay_read_events(void)
 
 void replay_init_events(void)
 {
-    replay_state.read_event_kind = -1;
+    replay_state.read_event_id = -1;
 }
 
 void replay_finish_events(void)
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index 59797c86cf..301131c1e6 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -12,6 +12,19 @@ 
  *
  */
 
+/* Asynchronous events IDs */
+
+typedef enum ReplayAsyncEventKind {
+    REPLAY_ASYNC_EVENT_BH,
+    REPLAY_ASYNC_EVENT_BH_ONESHOT,
+    REPLAY_ASYNC_EVENT_INPUT,
+    REPLAY_ASYNC_EVENT_INPUT_SYNC,
+    REPLAY_ASYNC_EVENT_CHAR_READ,
+    REPLAY_ASYNC_EVENT_BLOCK,
+    REPLAY_ASYNC_EVENT_NET,
+    REPLAY_ASYNC_COUNT
+} ReplayAsyncEventKind;
+
 /* Any changes to order/number of events will need to bump REPLAY_VERSION */
 enum ReplayEvents {
     /* for instruction event */
@@ -22,6 +35,7 @@  enum ReplayEvents {
     EVENT_EXCEPTION,
     /* for async events */
     EVENT_ASYNC,
+    EVENT_ASYNC_LAST = EVENT_ASYNC + REPLAY_ASYNC_COUNT - 1,
     /* for shutdown requests, range allows recovery of ShutdownCause */
     EVENT_SHUTDOWN,
     EVENT_SHUTDOWN_LAST = EVENT_SHUTDOWN + SHUTDOWN_CAUSE__MAX,
@@ -49,21 +63,6 @@  enum ReplayEvents {
     EVENT_COUNT
 };
 
-/* Asynchronous events IDs */
-
-enum ReplayAsyncEventKind {
-    REPLAY_ASYNC_EVENT_BH,
-    REPLAY_ASYNC_EVENT_BH_ONESHOT,
-    REPLAY_ASYNC_EVENT_INPUT,
-    REPLAY_ASYNC_EVENT_INPUT_SYNC,
-    REPLAY_ASYNC_EVENT_CHAR_READ,
-    REPLAY_ASYNC_EVENT_BLOCK,
-    REPLAY_ASYNC_EVENT_NET,
-    REPLAY_ASYNC_COUNT
-};
-
-typedef enum ReplayAsyncEventKind ReplayAsyncEventKind;
-
 typedef struct ReplayState {
     /*! Cached clock values. */
     int64_t cached_clock[REPLAY_CLOCK_COUNT];
@@ -83,8 +82,6 @@  typedef struct ReplayState {
     uint64_t block_request_id;
     /*! Prior value of the host clock */
     uint64_t host_clock_last;
-    /*! Asynchronous event type read from the log */
-    int32_t read_event_kind;
     /*! Asynchronous event id read from the log */
     uint64_t read_event_id;
 } ReplayState;
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 7e935deb15..10a7cf7992 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -59,7 +59,6 @@  static const VMStateDescription vmstate_replay = {
         VMSTATE_UINT32(has_unread_data, ReplayState),
         VMSTATE_UINT64(file_offset, ReplayState),
         VMSTATE_UINT64(block_request_id, ReplayState),
-        VMSTATE_INT32(read_event_kind, ReplayState),
         VMSTATE_UINT64(read_event_id, ReplayState),
         VMSTATE_END_OF_LIST()
     },
diff --git a/replay/replay.c b/replay/replay.c
index ccd7edec76..4c396bb376 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -22,7 +22,7 @@ 
 
 /* Current version of the replay mechanism.
    Increase it when file format changes. */
-#define REPLAY_VERSION              0xe0200b
+#define REPLAY_VERSION              0xe0200c
 /* Size of replay log header */
 #define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
 
@@ -221,7 +221,8 @@  bool replay_has_event(void)
         replay_account_executed_instructions();
         res = EVENT_CHECKPOINT <= replay_state.data_kind
               && replay_state.data_kind <= EVENT_CHECKPOINT_LAST;
-        res = res || replay_state.data_kind == EVENT_ASYNC;
+        res = res || (EVENT_ASYNC <= replay_state.data_kind
+                     && replay_state.data_kind <= EVENT_ASYNC_LAST);
     }
     return res;
 }