Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Nilay Vaish]

On 23 June 2016 at 09:31, pi3orama  wrote:
>
>
> 发自我的 iPhone
>
>> 在 2016年6月23日，下午10:27，Nilay Vaish  写道：
>>
>>> On 23 June 2016 at 00:27, Wang Nan  wrote:
>>> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = 
>>> {
>>>.type = PERF_RECORD_FINISHED_ROUND,
>>> };
>>>
>>> +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;
>>
>>
>> Wang, thanks for making the changes I suggested.
>> The patch overall looks fine to me.  Just as a matter
>> of style, I probably would not write case labels that
>> do not have any statements associated with them.
>> I'll let default take care of those labels.
>>
>
> I don't like them either.
>
> You have to do this when you start working
> on perf code. It turns on -Wall and -Werror,
> without these case gcc will complain,
> compiling will fail.
>

Oops!  Did not realize that compiler would complain.
I guess we have to leave them as they are.  Patch is good to be committed.

Thanks
Nilay

Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Nilay Vaish]

On 23 June 2016 at 09:31, pi3orama  wrote:
>
>
> 发自我的 iPhone
>
>> 在 2016年6月23日，下午10:27，Nilay Vaish  写道：
>>
>>> On 23 June 2016 at 00:27, Wang Nan  wrote:
>>> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = 
>>> {
>>>.type = PERF_RECORD_FINISHED_ROUND,
>>> };
>>>
>>> +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;
>>
>>
>> Wang, thanks for making the changes I suggested.
>> The patch overall looks fine to me.  Just as a matter
>> of style, I probably would not write case labels that
>> do not have any statements associated with them.
>> I'll let default take care of those labels.
>>
>
> I don't like them either.
>
> You have to do this when you start working
> on perf code. It turns on -Wall and -Werror,
> without these case gcc will complain,
> compiling will fail.
>

Oops!  Did not realize that compiler would complain.
I guess we have to leave them as they are.  Patch is good to be committed.

Thanks
Nilay

Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[pi3orama]

发自我的 iPhone

> 在 2016年6月23日，下午10:27，Nilay Vaish  写道：
> 
>> On 23 June 2016 at 00:27, Wang Nan  wrote:
>> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
>>.type = PERF_RECORD_FINISHED_ROUND,
>> };
>> 
>> +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;
> 
> 
> Wang, thanks for making the changes I suggested.
> The patch overall looks fine to me.  Just as a matter
> of style, I probably would not write case labels that
> do not have any statements associated with them.
> I'll let default take care of those labels.
> 

I don't like them either.

You have to do this when you start working
on perf code. It turns on -Wall and -Werror,
without these case gcc will complain,
compiling will fail.

Thank you.

> --
> Nilay

Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[pi3orama]

发自我的 iPhone

> 在 2016年6月23日，下午10:27，Nilay Vaish  写道：
> 
>> On 23 June 2016 at 00:27, Wang Nan  wrote:
>> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
>>.type = PERF_RECORD_FINISHED_ROUND,
>> };
>> 
>> +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;
> 
> 
> Wang, thanks for making the changes I suggested.
> The patch overall looks fine to me.  Just as a matter
> of style, I probably would not write case labels that
> do not have any statements associated with them.
> I'll let default take care of those labels.
> 

I don't like them either.

You have to do this when you start working
on perf code. It turns on -Wall and -Werror,
without these case gcc will complain,
compiling will fail.

Thank you.

> --
> Nilay

Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Nilay Vaish]

On 23 June 2016 at 00:27, Wang Nan  wrote:
> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
> .type = PERF_RECORD_FINISHED_ROUND,
>  };
>
> +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;


Wang, thanks for making the changes I suggested.
The patch overall looks fine to me.  Just as a matter
of style, I probably would not write case labels that
do not have any statements associated with them.
I'll let default take care of those labels.

--
Nilay

Re: [PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Nilay Vaish]

On 23 June 2016 at 00:27, Wang Nan  wrote:
> @@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
> .type = PERF_RECORD_FINISHED_ROUND,
>  };
>
> +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;


Wang, thanks for making the changes I suggested.
The patch overall looks fine to me.  Just as a matter
of style, I probably would not write case labels that
do not have any statements associated with them.
I'll let default take care of those labels.

--
Nilay

[PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Wang Nan]

overwrite_evt_state is introduced to reflect the state of overwritable
ring buffers. It is a state machine with 3 states:

.(forbid)_.
| |
| V
 RUNNING --(1)--> DATA_PENDING --(2)--> EMPTY
^  ^  |   ^   |
|  |__(forbid)/   |___(forbid)___/|
| |
 \_(3)___/

 RUNNING  : Overwritable ring buffers are recording
 DATA_PENDING : We are required to collect overwritable ring buffers
 EMPTY: We have collected data from those ring buffers.

 (1): Pause ring buffers for reading
 (2): Read from ring buffers
 (3): Resume ring buffers for recording

We can't avoid this complexity. Since we deliberately drop records from
overwritable ring buffer, there's no way for us to check remaining from
ring buffer itself (by checking head and old pointers). Therefore, we
need DATA_PENDING and EMPTY state to help us recording what we have done
to the ring buffer.

With the above state machine, this patch improves record__mmap_read_all(),
read from overwritable ring buffer when DATA_PENDING state is observed.

Signed-off-by: Wang Nan 
Signed-off-by: He Kuang 
Cc: Arnaldo Carvalho de Melo 
Cc: Jiri Olsa 
Cc: Masami Hiramatsu 
Cc: Namhyung Kim 
Cc: Zefan Li 
Cc: pi3or...@163.com
---
 tools/perf/builtin-record.c | 137 +++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d1873ee..ba19e9c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -42,6 +42,30 @@
 #include 
 #include 
 
+/*
+ * State machine of overwrite_evt_state:
+ *
+ *.(forbid)_.
+ *| V
+ * RUNNING --(1)--> DATA_PENDING --(2)--> EMPTY
+ *^  ^  |   ^   |
+ *|  |__(forbid)/   |___(forbid)___/|
+ *| |
+ * \_(3)___/
+ *
+ * RUNNING  : Overwritable ring buffers are recording
+ * DATA_PENDING : We are required to collect overwritable ring buffers
+ * EMPTY: We have collected data from those ring buffers.
+ *
+ * (1): Pause ring buffers for reading
+ * (2): Read from ring buffers
+ * (3): Resume ring buffers for recording
+ */
+enum overwrite_evt_state {
+   OVERWRITE_EVT_RUNNING,
+   OVERWRITE_EVT_DATA_PENDING,
+   OVERWRITE_EVT_EMPTY,
+};
 
 struct record {
struct perf_tooltool;
@@ -61,6 +85,7 @@ struct record {
boolbuildid_all;
booltimestamp_filename;
boolswitch_output;
+   enum overwrite_evt_state overwrite_evt_state;
unsigned long long  samples;
 };
 
@@ -462,6 +487,7 @@ try_again:
 
session->evlist = evlist;
perf_session__set_id_hdr_size(session);
+   rec->overwrite_evt_state = OVERWRITE_EVT_RUNNING;
 out:
return rc;
 }
@@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
.type = PERF_RECORD_FINISHED_ROUND,
 };
 
+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 

[PATCH v10 07/10] perf record: Read from overwritable ring buffer

[Wang Nan]

overwrite_evt_state is introduced to reflect the state of overwritable
ring buffers. It is a state machine with 3 states:

.(forbid)_.
| |
| V
 RUNNING --(1)--> DATA_PENDING --(2)--> EMPTY
^  ^  |   ^   |
|  |__(forbid)/   |___(forbid)___/|
| |
 \_(3)___/

 RUNNING  : Overwritable ring buffers are recording
 DATA_PENDING : We are required to collect overwritable ring buffers
 EMPTY: We have collected data from those ring buffers.

 (1): Pause ring buffers for reading
 (2): Read from ring buffers
 (3): Resume ring buffers for recording

We can't avoid this complexity. Since we deliberately drop records from
overwritable ring buffer, there's no way for us to check remaining from
ring buffer itself (by checking head and old pointers). Therefore, we
need DATA_PENDING and EMPTY state to help us recording what we have done
to the ring buffer.

With the above state machine, this patch improves record__mmap_read_all(),
read from overwritable ring buffer when DATA_PENDING state is observed.

Signed-off-by: Wang Nan 
Signed-off-by: He Kuang 
Cc: Arnaldo Carvalho de Melo 
Cc: Jiri Olsa 
Cc: Masami Hiramatsu 
Cc: Namhyung Kim 
Cc: Zefan Li 
Cc: pi3or...@163.com
---
 tools/perf/builtin-record.c | 137 +++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d1873ee..ba19e9c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -42,6 +42,30 @@
 #include 
 #include 
 
+/*
+ * State machine of overwrite_evt_state:
+ *
+ *.(forbid)_.
+ *| V
+ * RUNNING --(1)--> DATA_PENDING --(2)--> EMPTY
+ *^  ^  |   ^   |
+ *|  |__(forbid)/   |___(forbid)___/|
+ *| |
+ * \_(3)___/
+ *
+ * RUNNING  : Overwritable ring buffers are recording
+ * DATA_PENDING : We are required to collect overwritable ring buffers
+ * EMPTY: We have collected data from those ring buffers.
+ *
+ * (1): Pause ring buffers for reading
+ * (2): Read from ring buffers
+ * (3): Resume ring buffers for recording
+ */
+enum overwrite_evt_state {
+   OVERWRITE_EVT_RUNNING,
+   OVERWRITE_EVT_DATA_PENDING,
+   OVERWRITE_EVT_EMPTY,
+};
 
 struct record {
struct perf_tooltool;
@@ -61,6 +85,7 @@ struct record {
boolbuildid_all;
booltimestamp_filename;
boolswitch_output;
+   enum overwrite_evt_state overwrite_evt_state;
unsigned long long  samples;
 };
 
@@ -462,6 +487,7 @@ try_again:
 
session->evlist = evlist;
perf_session__set_id_hdr_size(session);
+   rec->overwrite_evt_state = OVERWRITE_EVT_RUNNING;
 out:
return rc;
 }
@@ -542,6 +568,79 @@ static struct perf_event_header finished_round_event = {
.type = PERF_RECORD_FINISHED_ROUND,
 };
 
+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;
+
+   switch (action)