Re: [PATCH v13 5/8] perf record: Read from overwritable ring buffer

From: Wangnan (F)
Date: Thu Jul 07 2016 - 01:00:46 EST




On 2016/7/6 20:34, Jiri Olsa wrote:
On Wed, Jul 06, 2016 at 08:03:28PM +0800, Wangnan (F) wrote:

On 2016/7/6 19:38, Jiri Olsa wrote:
On Mon, Jul 04, 2016 at 06:20:06AM +0000, Wang Nan wrote:

SNIP

+static void
+record__toggle_overwrite_evsels(struct record *rec,
+ enum overwrite_evt_state state)
+{
+ struct perf_evlist *evlist = rec->overwrite_evlist;
+ enum overwrite_evt_state old_state = rec->overwrite_evt_state;
+ enum action {
+ NONE,
+ PAUSE,
+ RESUME,
+ } action = NONE;
+
+ switch (old_state) {
+ case OVERWRITE_EVT_RUNNING: {
+ switch (state) {
+ case OVERWRITE_EVT_DATA_PENDING:
+ action = PAUSE;
+ break;
+ case OVERWRITE_EVT_RUNNING:
+ case OVERWRITE_EVT_EMPTY:
+ default:
+ goto state_err;
+ }
+ break;
+ }
+ case OVERWRITE_EVT_DATA_PENDING: {
+ switch (state) {
+ case OVERWRITE_EVT_EMPTY:
+ break;
+ case OVERWRITE_EVT_RUNNING:
+ case OVERWRITE_EVT_DATA_PENDING:
+ default:
+ goto state_err;
+ }
+ break;
+ }
+ case OVERWRITE_EVT_EMPTY: {
+ switch (state) {
+ case OVERWRITE_EVT_RUNNING:
+ action = RESUME;
+ break;
+ case OVERWRITE_EVT_EMPTY:
+ case OVERWRITE_EVT_DATA_PENDING:
+ default:
+ goto state_err;
+ }
+ break;
+ }
+ default:
+ WARN_ONCE(1, "Shouldn't get there\n");
+ }
+
+ rec->overwrite_evt_state = state;
+
+ if (!evlist)
+ return;
I'd expect this check at the begining
I think even evlist is NULL the state changing is still required.
Actually, the state machine is independent with aux evlist. Even
we without overwritable evsels the state machine is still valid.
So let the state machine runs unconditionally.
hum, can't see that.. it's state machine to govern overwrite evlist, right?
if there's no overwrite evlist we should keep the current processing

Not as easy as I thought. Look at following code:

@@ -1006,8 +1122,27 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
}

if (trigger_is_hit(&switch_output_trigger)) {
+ /*
+ * If switch_output_trigger is hit, the data in
+ * overwritable ring buffer should have been collected,
+ * so overwrite_evt_state should be set to
+ * OVERWRITE_EVT_EMPTY.
+ *
+ * If SIGUSR2 raise after or during record__mmap_read_all(),
+ * record__mmap_read_all() didn't collect data from
+ * overwritable ring buffer. Read again.
+ */
+ if (rec->overwrite_evt_state == OVERWRITE_EVT_RUNNING)
+ continue;
trigger_ready(&switch_output_trigger);

+ /*
+ * Reenable events in overwrite ring buffer after
+ * record__mmap_read_all(): we should have collected
+ * data from it.
+ */
+ record__toggle_overwrite_evsels(rec, OVERWRITE_EVT_RUNNING);
+
if (!quiet)
fprintf(stderr, "[ perf record: dump data: Woken up %ld times ]\n",
waking);

Here perf tests whether reading from overwritable ring buffer is required.
If SIGUSR2 is received just before the above trigger_is_hit, we should read from
overwrite ring buffer again. The OVERWRITE_EVT_RUNNING checker is for this reason.

Now if we stop the state machine, the state is stopped at OVERWRITE_EVT_RUNNING,
causes perf loops forever.

We can check rec->overwrite_evlist first, but it is ugly, since I believe the
overwritable state is independent to overwrite evlist. So I decide to introduce
a new state indicate the overwrite evlist is not ready.

Thank you.


if it's meant to govern the mmap reading in general
we should at least rename it
jirka