Patchwork [2/4] trace: Direct access of atomics is verboten, use the API

login
register
mail settings
Submitter Markus Armbruster
Date Jan. 25, 2013, 3:43 p.m.
Message ID <1359128620-14226-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/215789/
State New
Headers show

Comments

Markus Armbruster - Jan. 25, 2013, 3:43 p.m.
The GLib Reference Manual says:

    It is very important that all accesses to a particular integer or
    pointer be performed using only this API and that different sizes
    of operation are not mixed or used on overlapping memory
    regions. Never read or assign directly from or to a value --
    always use this API.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 trace/simple.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Laszlo Ersek - Jan. 31, 2013, 11:25 a.m.
On 01/25/13 16:43, Markus Armbruster wrote:
> The GLib Reference Manual says:
> 
>     It is very important that all accesses to a particular integer or
>     pointer be performed using only this API and that different sizes
>     of operation are not mixed or used on overlapping memory
>     regions. Never read or assign directly from or to a value --
>     always use this API.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  trace/simple.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Nothing beats a patch summary with a word that English loans from the
submitter's mother tongue! :) Extra points for the Vau.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Harsh Prateek Bora - Jan. 31, 2013, 11:59 a.m.
On 01/25/2013 09:13 PM, Markus Armbruster wrote:
> The GLib Reference Manual says:
>
>      It is very important that all accesses to a particular integer or
>      pointer be performed using only this API and that different sizes
>      of operation are not mixed or used on overlapping memory
>      regions. Never read or assign directly from or to a value --
>      always use this API.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

> ---
>   trace/simple.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/trace/simple.c b/trace/simple.c
> index ccbdb6a..592ff48 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -166,13 +166,13 @@ static gpointer writeout_thread(gpointer opaque)
>       for (;;) {
>           wait_for_trace_records_available();
>
> -        if (dropped_events) {
> +        if (g_atomic_int_get(&dropped_events)) {
>               dropped.rec.event = DROPPED_EVENT_ID,
>               dropped.rec.timestamp_ns = get_clock();
>               dropped.rec.length = sizeof(TraceRecord) + sizeof(uint64_t),
>               dropped.rec.reserved = 0;
>               while (1) {
> -                dropped_count = dropped_events;
> +                dropped_count = g_atomic_int_get(&dropped_events);
>                   if (g_atomic_int_compare_and_exchange(&dropped_events,
>                                                         dropped_count, 0)) {
>                       break;
> @@ -214,7 +214,7 @@ int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasi
>       uint64_t timestamp_ns = get_clock();
>
>       while (1) {
> -        old_idx = trace_idx;
> +        old_idx = g_atomic_int_get(&trace_idx);
>           smp_rmb();
>           new_idx = old_idx + rec_len;
>
> @@ -275,7 +275,8 @@ void trace_record_finish(TraceBufferRecord *rec)
>       record.event |= TRACE_RECORD_VALID;
>       write_to_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
>
> -    if ((trace_idx - writeout_idx) > TRACE_BUF_FLUSH_THRESHOLD) {
> +    if ((g_atomic_int_get(&trace_idx) - writeout_idx)
> +        > TRACE_BUF_FLUSH_THRESHOLD) {
>           flush_trace_file(false);
>       }
>   }
>

Patch

diff --git a/trace/simple.c b/trace/simple.c
index ccbdb6a..592ff48 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -166,13 +166,13 @@  static gpointer writeout_thread(gpointer opaque)
     for (;;) {
         wait_for_trace_records_available();
 
-        if (dropped_events) {
+        if (g_atomic_int_get(&dropped_events)) {
             dropped.rec.event = DROPPED_EVENT_ID,
             dropped.rec.timestamp_ns = get_clock();
             dropped.rec.length = sizeof(TraceRecord) + sizeof(uint64_t),
             dropped.rec.reserved = 0;
             while (1) {
-                dropped_count = dropped_events;
+                dropped_count = g_atomic_int_get(&dropped_events);
                 if (g_atomic_int_compare_and_exchange(&dropped_events,
                                                       dropped_count, 0)) {
                     break;
@@ -214,7 +214,7 @@  int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasi
     uint64_t timestamp_ns = get_clock();
 
     while (1) {
-        old_idx = trace_idx;
+        old_idx = g_atomic_int_get(&trace_idx);
         smp_rmb();
         new_idx = old_idx + rec_len;
 
@@ -275,7 +275,8 @@  void trace_record_finish(TraceBufferRecord *rec)
     record.event |= TRACE_RECORD_VALID;
     write_to_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
 
-    if ((trace_idx - writeout_idx) > TRACE_BUF_FLUSH_THRESHOLD) {
+    if ((g_atomic_int_get(&trace_idx) - writeout_idx)
+        > TRACE_BUF_FLUSH_THRESHOLD) {
         flush_trace_file(false);
     }
 }