logger: Change logger_offset() from macro to function

Make this code slightly easier to read, and eliminate calls to sub-routines.  Some of these were previously optimized away by the compiler, but one memcpy was not.

In my testing, this makes the code about 20% smaller, and has no sub-routine calls and no branches (on ARM).

v2 of this patch is, IMHO, easier to read than v1. Compared to that patch it uses __u8 instead of unsigned char, for consistency with the __u16 val data type, simplifies the conditional expression, adds a another comment, and moves a common statement out of the if.

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |   20 ++++++++++++--------

 1 files changed, 12 insertions(+), 8 deletions(-)

drivers/staging/android/logger.c +0 -0

index 92456d7..3475cb7 100644
@@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
  * get_entry_len - Grabs the length of the payload of the next entry starting
  * from 'off'.

drivers/staging/android/logger.c +0 -4

-       switch (log->size - off) {
-       case 1:
-               memcpy(&val, log->buffer + off, 1);
-               memcpy(((char *) &val) + 1, log->buffer, 1);

drivers/staging/android/logger.c +2 -1

+       else
+               ((__u8 *)&val)[1] = log->buffer[0];
 
        return sizeof(struct logger_entry) + val;
  }
--

1.7.2.3

Quoted 25 lines · Tim Bird

Make this code slightly easier to read, and eliminate calls to sub-routines. Some of these were previously optimized away by the compiler, but one memcpy was not.

In my testing, this makes the code about 20% smaller, and has no sub-routine calls and no branches (on ARM).

v2 of this patch is, IMHO, easier to read than v1. Compared to that patch it uses __u8 instead of unsigned char, for consistency with the __u16 val data type, simplifies the conditional expression, adds a another comment, and moves a common statement out of the if.

Signed-off-by Tim Bird<[email protected]>

---
  drivers/staging/android/logger.c |   20 ++++++++++++--------
  1 files changed, 12 insertions(+), 8 deletions(-)

drivers/staging/android/logger.c +0 -0

index 92456d7..3475cb7 100644
@@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
  * get_entry_len - Grabs the length of the payload of the next entry starting
  * from 'off'.

drivers/staging/android/logger.c static inline struct logger_log *file_get_log(struct file *file) · line 104

Quoted 10 lines

drivers/staging/android/logger.c +4 -0

  *
+ * An entry length is 2 bytes (16 bits) in host endian order.
+ * In the log, the length does not include the size of the log entry structure.
+ * This function returns the size including the log entry structure.
+ *
  * Caller needs to hold log->mutex.
  */
  static __u32 get_entry_len(struct logger_log *log, size_t off)
  {
        __u16 val;

Could using a union here make things look a little cleaner in the meat of the function? Something like

union {
     __u16 s;
     __u8 b[2];
} val;

Quoted 4 lines

drivers/staging/android/logger.c +0 -4

-       switch (log->size - off) {
-       case 1:
-               memcpy(&val, log->buffer + off, 1);
-               memcpy(((char *)&val) + 1, log->buffer, 1);

drivers/staging/android/logger.c static inline struct logger_log *file_get_log(struct file *file) · line 111

Quoted 10 lines

drivers/staging/android/logger.c +6 -4

-               break;
-       default:
-               memcpy(&val, log->buffer + off, 2);
-       }
+       /* copy 2 bytes from buffer, in memcpy order, */
+       /* handling possible wrap at end of buffer */
+
+       ((__u8 *)&val)[0] = log->buffer[off];
+       if (likely(off+1<  log->size))
+               ((__u8 *)&val)[1] = log->buffer[off+1];

spaces around the + in 'off+1' in the above two lines.

If mutex_lock waits, it will return in state TASK_RUNNING, rubbing out the effect of prepare_to_wait().

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |    3 ++-

 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/staging/android/logger.c +0 -0

index 92cfd94..54b7cdf 100644
@@ -172,9 +172,10 @@ static ssize_t logger_read(struct file *file, char __user *buf,
 

 start:
       while (1) {
+               mutex_lock(&log->mutex);
+
               prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);

-               mutex_lock(&log->mutex); ret = (log->w_off == reader->r_off); mutex_unlock(&log->mutex); if (!ret)
--
1.7.2.3

Add commentary, rename the function and make the code easier to read.

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |   28 ++++++++++++++++++++--------

 1 files changed, 20 insertions(+), 8 deletions(-)

drivers/staging/android/logger.c +20 -9

index 54b7cdf..8d9d4f1 100644
@@ -242,16 +242,28 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
  }
 
  /*
- * clock_interval - is a < c < b in mod-space? Put another way, does the line
- * from a to b cross c?
+ * is_between - is a < c < b, accounting for wrapping of a, b, and c
+ *    positions in the buffer
+ *
+ * That is, if a<b, check for c between a and b
+ * and if a>b, check for c outside (not between) a and b
+ *
+ * |------- a xxxxxxxx b --------|
+ *               c^
+ *
+ * |xxxxx b --------- a xxxxxxxxx|
+ *    c^
+ *  or                    c^
  */
-static inline int clock_interval(size_t a, size_t b, size_t c)
+static inline int is_between(size_t a, size_t b, size_t c)
  {
-       if (b < a) {
-               if (a < c || b >= c)
+       if (a < b) {
+               /* is c between a and b? */
+               if (a < c && c <= b)
                        return 1;
        } else {
-               if (a < c && b >= c)
+               /* is c outside of b through a? */
+               if (c <= b || a < c)
                        return 1;
        }
 

@@ -272,11 +284,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
        size_t new = logger_offset(log, old + len);
        struct logger_reader *reader;
 
-       if (clock_interval(old, new, log->head))
+       if (is_between(old, new, log->head))
                log->head = get_next_entry(log, log->head, len);
 
        list_for_each_entry(reader, &log->readers, list)
-               if (clock_interval(old, new, reader->r_off))
+               if (is_between(old, new, reader->r_off))
                        reader->r_off = get_next_entry(log, reader->r_off, len);
  }
 
--

1.7.2.3

Add comment to explain when w_off is not updated in case of failed second fragment copy to buffer.

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/staging/android/logger.c +6 -1

index 8d9d4f1..115d8ed 100644
@@ -330,6 +330,12 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
 
        if (count != len)
                if (copy_from_user(log->buffer, buf + len, count - len))
+                       /*
+                        * Note that by not updating w_off, this abandons the
+                        * portion of the new entry that *was* successfully
+                        * copied, just above.  This is intentional to avoid
+                        * message corruption from missing fragments.
+                        */
                        return -EFAULT;
 
        log->w_off = logger_offset(log, log->w_off + count);
--

1.7.2.3

Convert to function and add log as a parameter, rather than relying on log in the context of the macro.

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

drivers/staging/android/logger.c +0 -0

index ffc2d04..92456d7 100644

Convert to function and add log as a parameter, rather than relying on log in the context of the macro.

Signed-off-by Tim Bird <[email protected]>

---
 drivers/staging/android/logger.c |   16 ++++++++++------

 1 files changed, 10 insertions(+), 6 deletions(-)

drivers/staging/android/logger.c +0 -0

index ffc2d04..92456d7 100644

drivers/staging/android/logger.c +7 -6

+}
+
 
  /*
  * file_get_log - Given a file structure, return the associated log

@@ -137,7 +141,7 @@ static ssize_t do_read_log_to_user(struct logger_log *log,
                if (copy_to_user(buf + len, log->buffer, count - len))
                        return -EFAULT;
 
-       reader->r_off = logger_offset(reader->r_off + count);
+       reader->r_off = logger_offset(log, reader->r_off + count);
 
        return count;
  }

@@ -225,7 +229,7 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
 
        do {
                size_t nr = get_entry_len(log, off);
-               off = logger_offset(off + nr);
+               off = logger_offset(log, off + nr);
                count += nr;
        } while (count < len);
 

@@ -260,7 +264,7 @@ static inline int clock_interval(size_t a, size_t b, size_t c)
  static void fix_up_readers(struct logger_log *log, size_t len)
  {
        size_t old = log->w_off;
-       size_t new = logger_offset(old + len);
+       size_t new = logger_offset(log, old + len);
        struct logger_reader *reader;
 
        if (clock_interval(old, new, log->head))

@@ -286,7 +290,7 @@ static void do_write_log(struct logger_log *log, const void *buf, size_t count)
        if (count != len)
                memcpy(log->buffer, buf + len, count - len);
 
-       log->w_off = logger_offset(log->w_off + count);
+       log->w_off = logger_offset(log, log->w_off + count);
 
  }
 

@@ -311,7 +315,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
                if (copy_from_user(log->buffer, buf + len, count - len))
                        return -EFAULT;
 
-       log->w_off = logger_offset(log->w_off + count);
+       log->w_off = logger_offset(log, log->w_off + count);
 
        return count;
  }
--

1.7.2.3