Skip to contentSkip to conversation

staging: android: logger: Allocate logs dynamically at boot

PATCH In review Tim Bird 3 participants 8 messages May 4, 2012 – May 7, 2012 Staging

lore
Share & tools
lore

Tim Bird

author
#0 lore raw

PATCH 1/1 staging: android: logger: Allocate logs dynamically at boot

This changes the log initialization to be dynamic, but still at boot time. These changes are a predecessor to implementing runtime allocation and freeing of logs, to make the Android logger less hard-coded.

This makes no ABI changes with user space.

Signed-off-by Tim Bird <[email protected]>
---
 drivers/staging/android/logger.c |  117 ++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 42 deletions(-)
drivers/staging/android/logger.c +75 -42
index ea69b6a..fa5fd71 100644
@@ -25,6 +25,7 @@
 #include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/time.h>
+#include <linux/vmalloc.h>
 #include "logger.h"
 
 #include <asm/ioctls.h>

@@ -564,81 +565,113 @@ static const struct file_operations logger_fops = {
 	.release = logger_release,
 };
 
-/*
- * Defines a log structure with name 'NAME' and a size of 'SIZE' bytes, which
- * must be a power of two, greater than LOGGER_ENTRY_MAX_LEN, and less than
- * LONG_MAX minus LOGGER_ENTRY_MAX_LEN.
- */
-#define DEFINE_LOGGER_DEVICE(VAR, NAME, SIZE) \
-static unsigned char _buf_ ## VAR[SIZE]; \
-static struct logger_log VAR = { \
-	.buffer = _buf_ ## VAR, \
-	.misc = { \
-		.minor = MISC_DYNAMIC_MINOR, \
-		.name = NAME, \
-		.fops = &logger_fops, \
-		.parent = NULL, \
-	}, \
-	.wq = __WAIT_QUEUE_HEAD_INITIALIZER(VAR .wq), \
-	.readers = LIST_HEAD_INIT(VAR .readers), \
-	.mutex = __MUTEX_INITIALIZER(VAR .mutex), \
-	.w_off = 0, \
-	.head = 0, \
-	.size = SIZE, \
-};
-
-DEFINE_LOGGER_DEVICE(log_main, LOGGER_LOG_MAIN, 256*1024)
-DEFINE_LOGGER_DEVICE(log_events, LOGGER_LOG_EVENTS, 256*1024)
-DEFINE_LOGGER_DEVICE(log_radio, LOGGER_LOG_RADIO, 256*1024)
-DEFINE_LOGGER_DEVICE(log_system, LOGGER_LOG_SYSTEM, 256*1024)
+#define MAX_LOGS	5
+struct logger_log *logs_array[MAX_LOGS];
 
 static struct logger_log *get_log_from_minor(int minor)
 {
-	if (log_main.misc.minor == minor)
-		return &log_main;
-	if (log_events.misc.minor == minor)
-		return &log_events;
-	if (log_radio.misc.minor == minor)
-		return &log_radio;
-	if (log_system.misc.minor == minor)
-		return &log_system;
+	int i;
+
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i]->misc.minor == minor)
+			return logs_array[i];
+	}
 	return NULL;
 }
 
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
 {
-	int ret;
+	int i;
 
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i] == 0) {
+			logs_array[i] = log;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+/*
+ * Log size must be a power of two, greater than LOGGER_ENTRY_MAX_LEN,
+ * and less than LONG_MAX minus LOGGER_ENTRY_MAX_LEN.
+ */
+static int __init create_log(char *log_name, int size)
+{
+	int ret = 0;
+	struct logger_log *log;
+	unsigned char *buffer;
+
+	buffer = vmalloc(size);
+	if (buffer == NULL)
+		return -1;
+
+	log = kzalloc(sizeof(struct logger_log), GFP_KERNEL);
+	if (log == NULL) {
+		ret = -1;
+		goto out_free_buffer;
+	}
+	log->buffer = buffer;
+
+	log->misc.minor = MISC_DYNAMIC_MINOR;
+	log->misc.name = kstrdup(log_name, GFP_KERNEL);
+	if (log->misc.name == NULL) {
+		ret = -1;
+		goto out_free_log;
+	}
+
+	log->misc.fops = &logger_fops;
+	log->misc.parent = NULL;
+
+	init_waitqueue_head(&log->wq);
+	INIT_LIST_HEAD(&log->readers);
+	mutex_init(&log->mutex);
+	log->w_off = 0;
+	log->head = 0;
+	log->size = size;
+
+	ret = add_log(log);
+	if (unlikely(ret))
+		goto out_free_log;
+
+	/* finally, initialize the misc device for this log */
 	ret = misc_register(&log->misc);
 	if (unlikely(ret)) {
 		printk(KERN_ERR "logger: failed to register misc "
 		       "device for log '%s'!\n", log->misc.name);
-		return ret;
+		goto out_free_log;
 	}
 
 	printk(KERN_INFO "logger: created %luK log '%s'\n",
 	       (unsigned long) log->size >> 10, log->misc.name);
 
 	return 0;
+
+out_free_log:
+	kfree(log);
+
+out_free_buffer:
+	vfree(buffer);
+	return ret;
 }
 
 static int __init logger_init(void)
 {
 	int ret;
 
-	ret = init_log(&log_main);
+	ret = create_log(LOGGER_LOG_MAIN, 256*1024);
 	if (unlikely(ret))
 		goto out;
 
-	ret = init_log(&log_events);
+	ret = create_log(LOGGER_LOG_EVENTS, 256*1024);
 	if (unlikely(ret))
 		goto out;
 
-	ret = init_log(&log_radio);
+	ret = create_log(LOGGER_LOG_RADIO, 256*1024);
 	if (unlikely(ret))
 		goto out;
 
-	ret = init_log(&log_system);
+	ret = create_log(LOGGER_LOG_SYSTEM, 256*1024);
 	if (unlikely(ret))
 		goto out;
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw

drivers/staging/android/logger.c static const struct file_operations logger_fops = { · line 569

Quoted 2 lines · Tim Bird
drivers/staging/android/logger.c +2 -0
+#define MAX_LOGS	5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Quoted 4 lines
drivers/staging/android/logger.c +1 -2
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
 {
-	int ret;

drivers/staging/android/logger.c static const struct file_operations logger_fops = { · line 593

Quoted 10 lines
drivers/staging/android/logger.c +9 -0
+	int i;
 
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i] == 0) {
+			logs_array[i] = log;
+			return 0;
+		}
+	}
+	return -1;
+}

I see you didn't run your patch through sparse :(

Care to fix up the sparse warnings and resend?

thanks,

greg k-h

author
#2 lore raw
Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Tim Bird
(quoted diff) +2 -0
+#define MAX_LOGS	5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Would that be better? The for-loop is IMHO simpler than a list walk for finding matches. I anticipate that the size of this array should never (famous last words) be bigger than about 20 entries, even in the dynamic-allocation-per-application case. And that's a ways off in implementation.

I'll be happy to switch to a linked list once it looks like we're going to have more than 5 entries. Or I can switch to a linked list now if you think it's better form to code for the longer-term anticipated features.

Not a big deal to me either way.

Quoted 7 lines
Quoted 7 lines
(quoted diff) +2 -2
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
 {
-	int ret;
+	int i;
Quoted 10 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i] == 0) {
+			logs_array[i] = log;
+			return 0;
+		}
+	}
+	return -1;
+}

I see you didn't run your patch through sparse :(

Indeed - shame on me. :-(

Quoted 1 line

Care to fix up the sparse warnings and resend?

OK - this patch has 2 sparse issues, and there were 2 already in the code. Do you want me to send the other 2 sparse fixes as an independent patch, or could I cheat and throw them in this one?

Below is what the fixes look like. (BTW - these are formatted for human review but not for mainline submission.)

Thanks for your help.
-- Tim

drivers/staging/android/logger.c +4 -4
index fa5fd71..9f4ed84 100644
@@ -61,7 +61,7 @@ struct logger_reader {
 };
 
 /* logger_offset - returns index 'n' into the log via (optimized) modulus */
-size_t logger_offset(struct logger_log *log, size_t n)
+static size_t logger_offset(struct logger_log *log, size_t n)
 {
        return n & (log->size-1);
 }

@@ -349,7 +349,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
  * writev(), and aio_write(). Writes are our fast path, and we try to optimize
  * them above all else.
  */
-ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
+static ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
                         unsigned long nr_segs, loff_t ppos)
 {
        struct logger_log *log = file_get_log(iocb->ki_filp);

@@ -566,7 +566,7 @@ static const struct file_operations logger_fops = {
 };
 
 #define MAX_LOGS       5
-struct logger_log *logs_array[MAX_LOGS];
+static struct logger_log *logs_array[MAX_LOGS];
 
 static struct logger_log *get_log_from_minor(int minor)
 {

@@ -584,7 +584,7 @@ static int __init add_log(struct logger_log *log)
        int i;
 
        for (i = 0; i < MAX_LOGS; i++) {
-               if (logs_array[i] == 0) {
+               if (logs_array[i] == NULL) {
                        logs_array[i] = log;
                        return 0;
                }

=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation Senior Staff Engineer, Sony Network Entertainment
=============================

maintainer
#3 lore raw
Quoted 14 lines · Tim Bird
Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Tim Bird
(quoted diff) +2 -0
+#define MAX_LOGS	5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Would that be better? The for-loop is IMHO simpler than a list walk for finding matches. I anticipate that the size of this array should never (famous last words) be bigger than about 20 entries, even in the dynamic-allocation-per-application case. And that's a ways off in implementation.

Quoted 6 lines

I'll be happy to switch to a linked list once it looks like we're going to have more than 5 entries. Or I can switch to a linked list now if you think it's better form to code for the longer-term anticipated features.

Not a big deal to me either way.

For now, it's fine, just in the future I imagine, if you are going to continue down the patch of this type of api and interface, you will want to make it dynamic.

Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +2 -2
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
 {
-	int ret;
+	int i;
Quoted 10 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i] == 0) {
+			logs_array[i] = log;
+			return 0;
+		}
+	}
+	return -1;
+}

I see you didn't run your patch through sparse :(

Quoted 7 lines

Indeed - shame on me. :-(

Quoted 1 line

Care to fix up the sparse warnings and resend?

OK - this patch has 2 sparse issues, and there were 2 already in the code. Do you want me to send the other 2 sparse fixes as an independent patch, or could I cheat and throw them in this one?

Fix your patch first and resend it. You can send the 'fix up sparse warnings' patch after it if you want to.

I don't want to add new patches that add new checkpatch and sparse problems, the goal of having the code in the staging tree is that it gets better over time, not worse :)

thanks,

greg k-h

author
#4 lore raw
Quoted 19 lines · Greg KH
Quoted 18 lines · Tim Bird
Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Tim Bird
(quoted diff) +2 -0
+#define MAX_LOGS	5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Would that be better? The for-loop is IMHO simpler than a list walk for finding matches. I anticipate that the size of this array should never (famous last words) be bigger than about 20 entries, even in the dynamic-allocation-per-application case. And that's a ways off in implementation.

I'll be happy to switch to a linked list once it looks like we're going to have more than 5 entries. Or I can switch to a linked list now if you think it's better form to code for the longer-term anticipated features.

Quoted 5 lines
Quoted 1 line

Not a big deal to me either way.

For now, it's fine, just in the future I imagine, if you are going to continue down the patch of this type of api and interface, you will want to make it dynamic.

I'm still working out how this might integrate into the syslog API (and with Kay's recent work). It's unclear if this will be the ultimate data structure holding these entries or not - or if something different will take its place. In any event, I'll keep this in mind if we stay with this data structure.

Quoted 20 lines
Quoted 20 lines
Quoted 7 lines
Quoted 7 lines
(quoted diff) +2 -2
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
 {
-	int ret;
+	int i;
Quoted 10 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+	for (i = 0; i < MAX_LOGS; i++) {
+		if (logs_array[i] == 0) {
+			logs_array[i] = log;
+			return 0;
+		}
+	}
+	return -1;
+}

I see you didn't run your patch through sparse :(

Indeed - shame on me. :-(

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Care to fix up the sparse warnings and resend?

OK - this patch has 2 sparse issues, and there were 2 already in the code. Do you want me to send the other 2 sparse fixes as an independent patch, or could I cheat and throw them in this one?

Fix your patch first and resend it. You can send the 'fix up sparse warnings' patch after it if you want to.

OK - will do.

Quoted 3 lines

I don't want to add new patches that add new checkpatch and sparse problems, the goal of having the code in the staging tree is that it gets better over time, not worse :)

hehe. Yes. :-)

BTW, I have a whitespace cleanup for one of my previous patches. I'm unsure if I should send this in, or wait until the code in question is affected by another patch. I thought the standard kernel policy was to not do standalone whitespace cleanup patches (especially trivial ones). But is that the same policy for staging?

I'm happy to submit something now, or wait for a more appropriate time in the future, and just watch for the right opportunity.
-- Tim

=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation Senior Staff Engineer, Sony Network Entertainment
=============================

maintainer
#5 lore raw
Quoted 5 lines · Tim Bird

BTW, I have a whitespace cleanup for one of my previous patches. I'm unsure if I should send this in, or wait until the code in question is affected by another patch. I thought the standard kernel policy was to not do standalone whitespace cleanup patches (especially trivial ones). But is that the same policy for staging?

No, the policy is to do stand-alone whitespace cleanup patches, especially trivial ones :)

Well, for portions of the kernel I maintain at least, I don't mind them at all, send them on when you have them.

thanks,

greg k-h

#6 lore raw
In reply to Tim Bird
Quoted 14 lines · Tim Bird
Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Tim Bird
(quoted diff) +2 -0
+#define MAX_LOGS    5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Would that be better?  The for-loop is IMHO simpler than a list walk for finding matches.  I anticipate that the size of this array should never (famous last words) be bigger than about 20 entries, even in the dynamic-allocation-per-application case.  And that's a ways off in implementation.

Quoted 6 lines

I'll be happy to switch to a linked list once it looks like we're going to have more than 5 entries.  Or I can switch to a linked list now if you think it's better form to code for the longer-term anticipated features.

Not a big deal to me either way.

I'd also favor a list approach. It won't break unexpectedly when somebody inevitably adds that 6th log or whatever, and the list traversal cost only occurs during open(), which is not a frequent operation -- most users keep the log driver open for the process lifespan (both readers and writers).

Brian

author
#7 lore raw
Quoted 20 lines · Brian Swetland
Quoted 19 lines · Tim Bird
Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Tim Bird
(quoted diff) +2 -0
+#define MAX_LOGS    5
+struct logger_log *logs_array[MAX_LOGS];

You are going to make this a list and not a static array in the future, right?

Would that be better? The for-loop is IMHO simpler than a list walk for finding matches. I anticipate that the size of this array should never (famous last words) be bigger than about 20 entries, even in the dynamic-allocation-per-application case. And that's a ways off in implementation.

I'll be happy to switch to a linked list once it looks like we're going to have more than 5 entries. Or I can switch to a linked list now if you think it's better form to code for the longer-term anticipated features.

Quoted 7 lines
Quoted 1 line

Not a big deal to me either way.

I'd also favor a list approach. It won't break unexpectedly when somebody inevitably adds that 6th log or whatever, and the list traversal cost only occurs during open(), which is not a frequent operation -- most users keep the log driver open for the process lifespan (both readers and writers).

OK - I'll convert to a list and send an updated patch soon.
-- Tim

=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation Senior Staff Engineer, Sony Network Entertainment
=============================