* Re: RFC: android logger feedback request
From: NeilBrown @ 2011-12-22 7:05 UTC (permalink / raw)
To: Brian Swetland
Cc: david, Tim Bird, Greg KH, linux-embedded, linux kernel,
Arnd Bergmann, john stultz, Kay Sievers, Lennart Poettering
In-Reply-To: <CANqkERBPayBWjFp2nCDZA2CByJQwHMoeXe62ZutZr8QkWtcTLg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4613 bytes --]
On Wed, 21 Dec 2011 21:06:02 -0800 Brian Swetland <swetland@google.com> wrote:
> On Wed, Dec 21, 2011 at 8:52 PM, <david@lang.hm> wrote:
> > On Thu, 22 Dec 2011, NeilBrown wrote:
> >>
> >> You would defined 'read' and 'write' much like you currently do to create
> >> a list of
> >> datagrams in a circular buffer and replace the ioctls by more standard
> >> interfaces:
> >>
> >> LOGGER_GET_LOG_BUG_SIZE would use 'stat' and the st_blocks field
> >> LOGGER_GET_LOG_LEN would use 'stat' and the st_size field
> >> LOGGER_GET_NEXT_ENTRY_LEN could use the FIONREAD ioctl
> >> LOGGER_FLUSH_LOG could use ftruncate
> >>
> >> The result would be much the same amount of code, but an interface which
> >> has
> >> fewer details hard-coded and is generally more versatile and accessible.
> >
> > That sounds better than what has been done in android, but it is still _far_
> > more limited than doing something that could be replaced by a fairly
> > standard syslog daemon.
>
> We're really not interested in adding another daemon to the platform
> -- the logging system we have has served us well, is integrated with
> our existing development tools, and we're definitely interested in
> improving it, but throwing it out and replacing it with a userspace
> solution is not interesting to us right now.
>
> Brian
Possibly it would be useful to be clear what we all *are* really interested
in, because I suspect there is a lot of variety.
You appear to be interested in providing a great platform for a phone, in
minimising unnecessary churn in that platform, and in having the freedom to
optimise various aspects however you see fit. You appear to have little
problem with maintaining some components out-of-mainline. This is all
perfectly valid and very much in the spirit of "freedom" that binds us
together.
Others want to be able to run a main-line kernel underneath the Android
user-space - and that is perfectly reasonable as well.
I don't much care about either of those, but I want "Linux" to be "high
quality" (according to my own personal standards of course) which means
keeping bad stuff out (though you could argue that the horse has well and
truly bolted there) and including good and useful stuff in. And I think
Android has something to offer on both sides there :-)
Weighing all that up, I don't think it is useful to set our goal on "getting
Android to use a mainline kernel" - that isn't going to happen.
Rather we should focus primarily on "making it *possible* to run android
user-space on mainline".
That could involve two things
1/ Making sure the interfaces that Android uses are abstracted at a suitable
level so that when running a mainline kernel you can just slip in an
alternate shared library with compatible functionality.
2/ Adding functionality to Linux so that it is possible to provide the
functionality that Android needs.
Android should not *have* to use the interface that the "mainline
community" decides is preferred, but nor should mainline be required to
include the drivers that Android wants to use. History shows us that isn't
going to happen.
But if there was a fairly low-level API that Android used, then those in the
community could who want Android on a mainline kernel could work to implement
that API with whatever mixture of user-space and kernel-space that achieves
consensus.
Android could of course change to use the community version eventually if
they found that was a good thing, or could keep using their own.
So: bringing this back to the android logger...
What I would like to see is a low-level API which is used to access logging
for which alternate implementations could be written. Ideally this would
be embodied in a single .so, but we have the source so that doesn't need to
be a barrier.
Then we could argue to our heart's content about how best to implement that
API - Journal and nsyslogd and rsyslogd could all implement it in different
ways and we could be one step closer to running Android on a mainline
kernel, but the Android developers don't need to be involved (but can if
they want to of course).
I would be important that the API is specified clearly - neither under
specified nor over-specified. That means that the Android implementation
would need to explicitly forbid anything that isn't explicit permitted.
This is because most testing will happen on the Android platform so it's
actual behaviour will become the defacto standard.
Could that be a possible way forward?
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Lennart Poettering @ 2011-12-22 13:40 UTC (permalink / raw)
To: Brian Swetland
Cc: Kay Sievers, Greg KH, Tim Bird, linux-embedded, linux kernel,
Arnd Bergmann, john stultz
In-Reply-To: <CANqkERCcciVxybh=66K5xxzzGmgw=fg0vwmcitWZ1y=DTxxtOA@mail.gmail.com>
On Wed, 21.12.11 20:22, Brian Swetland (swetland@google.com) wrote:
> > If we want to think about any next generation logging, I'm convinced,
> > we need to support records, structured data and binary data; anything
> > else is unlikely worth to change the current stuff.
>
> Any thoughts as to how one could allow N userspace agents to log to a
> single shared buffer without one agent, if buggy or malicious,
> spamming out all the other contents of the log? This is one of the
> main reasons we maintain separate logs in Android today, and are
> likely to continue doing so until we figure out how resolve the issue.
I have been working on some code to rate limit what clients can log to
the journal, with some nice tricks: the rate limiting is per-cgroup, so
that services can't flush out other services data so easily (assuming
they are in separate cgroups, which they are in a systemd world). Also,
different rates apply to to messages with different priorities
(i.e. we'll have a lower limit on debug messages, and allow more
important messages to be logged at a higher rate). And finally, we look
at the available disk space: the overall rate limits are increased if
there's more free space, and lowered if there's less.
> Also, as I mentioned earlier, from a security standpoint it is highly
> desirable to be able to restrict which records certain readers can
> read. Convincing third party app developers to not log sensitive or
> PII data remains a challenge -- providing the ability for filtered
> read access allows some mitigation (app can retrieve it's own logs for
> a bug report but can't sift through all logs looking for interesting
> tidbits).
The journal splits up user data into sparate files and sets file access
permissions to ensure that users can access their own data but nothing
else. However, when root wants to read all data it can, and the data
will be interleaved automatically.
Lennart
--
Lennart Poettering - Red Hat, Inc.
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Arnd Bergmann @ 2011-12-22 14:59 UTC (permalink / raw)
To: NeilBrown
Cc: Tim Bird, Greg KH, linux-embedded, linux kernel, john stultz,
Brian Swetland, Kay Sievers, Lennart Poettering
In-Reply-To: <20111222122026.3a0fae36@notabene.brown>
On Thursday 22 December 2011, NeilBrown wrote:
> If you created a 'logbuf' filesystem that used libfs to provide a single
> directory in which privileged processes could create files then you wouldn't
> need the kernel to "know" the allowed logs: radio, events, main, system.
> The size could be set by ftruncate() (by privileged used again) rather than
> being hardcoded.
>
> You would defined 'read' and 'write' much like you currently do to create a list of
> datagrams in a circular buffer and replace the ioctls by more standard
> interfaces:
>
> LOGGER_GET_LOG_BUG_SIZE would use 'stat' and the st_blocks field
> LOGGER_GET_LOG_LEN would use 'stat' and the st_size field
> LOGGER_GET_NEXT_ENTRY_LEN could use the FIONREAD ioctl
> LOGGER_FLUSH_LOG could use ftruncate
>
> The result would be much the same amount of code, but an interface which has
> fewer details hard-coded and is generally more versatile and accessible.
I like the idea and was going to suggest something very similar, but I wonder
if we could take the approach even further:
* Remove all kernel code for this and use a user space library together
with tmpfs
* prepopulate the tmpfs at boot time with all the log buffers in the right
size, and set the maximum file system size so that they cannot grow further.
* Have minimal formatting in the log buffer: A few bytes header (ring buffer
start and end)
* Mandate that user space must use mmap and atomic operations to reserve space
in the log and write to the files.
* Provide a tool to get the log data out of the buffer again in a race-free way.
Since any program that is allowed to write to the buffer can overwrite all
existing information in it anyway, I think we don't actually need any kernel
help in maintaining consistency of the contents either -- the reader will
simply discard any data. The main thing we would not be able to guarantee
without kernel help is proving the origin of individual messages, but I'm
not sure if that is a design goal.
Arnd
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Kay Sievers @ 2011-12-22 15:13 UTC (permalink / raw)
To: Arnd Bergmann
Cc: NeilBrown, Tim Bird, Greg KH, linux-embedded, linux kernel,
john stultz, Brian Swetland, Lennart Poettering
In-Reply-To: <201112221459.04701.arnd@arndb.de>
On Thu, Dec 22, 2011 at 15:59, Arnd Bergmann <arnd@arndb.de> wrote:
> * Remove all kernel code for this and use a user space library together
> with tmpfs
> * prepopulate the tmpfs at boot time with all the log buffers in the right
> size, and set the maximum file system size so that they cannot grow further.
> * Have minimal formatting in the log buffer: A few bytes header (ring buffer
> start and end)
> * Mandate that user space must use mmap and atomic operations to reserve space
> in the log and write to the files.
> * Provide a tool to get the log data out of the buffer again in a race-free way.
>
> Since any program that is allowed to write to the buffer can overwrite all
> existing information in it anyway, I think we don't actually need any kernel
> help in maintaining consistency of the contents either -- the reader will
> simply discard any data. The main thing we would not be able to guarantee
> without kernel help is proving the origin of individual messages, but I'm
> not sure if that is a design goal.
I'm very sure you want rate limiting, global sequence numbers and
trusted properties added to the log.
I can imagine that the kernel provides a single character device,
instantiates a tmpfs mountpoint and splits all stuff that travels into
that device into several files (ring buffers) in the tmpfs filesystem.
The files are probably just split by the uid of the incoming message
sender, which maps to an app at Android.
Only the character device would be able to write to the file, but the
user with the matching uid can read its own file back directly from
tmpfs.
I'm very convinced that we should not design a logging system today,
where we give the ownership of the data to the actual untrusted
process that logs.
The current kmsg buffer could just be one additional file in this tmpfs mount.
Kay
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Alan Cox @ 2011-12-23 15:22 UTC (permalink / raw)
To: Greg KH
Cc: Brian Swetland, david, NeilBrown, Tim Bird, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering
In-Reply-To: <20111222060917.GA16302@suse.de>
> I have no problem leaving the logger driver in staging, but it seems
> that Tim is taking on the task to do the harder thing here, which
> probably would entail work on both sides, which as a openhandset
> alliance company member, he might have a change that someone like me
> might not :)
I'd like to see it in staging. There is a general discussion going on
about how such logging services should look (and indeed it goes back
before Android bits), and it looks like a good way to proceed is going to
be to "play" with the Android one - perhaps adding the fs model to it.
So we need something and the Android interface at the very least is a
prototype that has production proof of working.
Alan
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Greg KH @ 2011-12-23 16:29 UTC (permalink / raw)
To: Alan Cox
Cc: Brian Swetland, david, NeilBrown, Tim Bird, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering
In-Reply-To: <20111223152231.4a304fdd@pyx>
On Fri, Dec 23, 2011 at 03:22:31PM +0000, Alan Cox wrote:
> > I have no problem leaving the logger driver in staging, but it seems
> > that Tim is taking on the task to do the harder thing here, which
> > probably would entail work on both sides, which as a openhandset
> > alliance company member, he might have a change that someone like me
> > might not :)
>
> I'd like to see it in staging. There is a general discussion going on
> about how such logging services should look (and indeed it goes back
> before Android bits), and it looks like a good way to proceed is going to
> be to "play" with the Android one - perhaps adding the fs model to it.
>
> So we need something and the Android interface at the very least is a
> prototype that has production proof of working.
It is already in staging, look in the staging-next branch of linux-next.
thanks,
greg k-h
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Andrew Morton @ 2011-12-29 0:39 UTC (permalink / raw)
To: Tim Bird
Cc: linux-embedded, linux kernel, Arnd Bergmann, john stultz, Greg KH,
Brian Swetland
In-Reply-To: <4EF264C3.6000104@am.sony.com>
On Wed, 21 Dec 2011 14:59:15 -0800
Tim Bird <tim.bird@am.sony.com> wrote:
> Hi all,
>
> I'm looking for feedback on the Android logger code,
The code looks nice.
>
> ...
>
> +/* logger_offset - returns index 'n' into the log via (optimized) modulus */
> +#define logger_offset(n) ((n) & (log->size - 1))
> +
This macro depends upon the presence of a local variable called "log"
and is therefore all stinky. Pass "log" in as an arg and convert it to
a regular C function.
>
> ...
>
> +/*
> + * get_entry_len - Grabs the length of the payload of the next entry starting
> + * from 'off'.
> + *
> + * Caller needs to hold log->mutex.
> + */
> +static __u32 get_entry_len(struct logger_log *log, size_t off)
> +{
> + __u16 val;
> +
> + switch (log->size - off) {
> + case 1:
> + memcpy(&val, log->buffer + off, 1);
> + memcpy(((char *) &val) + 1, log->buffer, 1);
So numbers in the buffer are in host endian order. That's worth
capturing in a comment somewhere.
There must be a way of doing the above more neatly, using
log->buffer[off] and log->buffer[0]. Perhaps using
include/linux/unaligned/packed_struct.h.
> + break;
> + default:
> + memcpy(&val, log->buffer + off, 2);
get_unaligned()
> + }
> +
> + return sizeof(struct logger_entry) + val;
> +}
> +
>
> ...
>
> +static ssize_t logger_read(struct file *file, char __user *buf,
> + size_t count, loff_t *pos)
> +{
> + struct logger_reader *reader = file->private_data;
> + struct logger_log *log = reader->log;
> + ssize_t ret;
> + DEFINE_WAIT(wait);
> +
> +start:
> + while (1) {
> + prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);
> +
> + mutex_lock(&log->mutex);
If mutex_lock() had to wait for the mutex, it will return in state
TASK_RUNNING, thus rubbing out the effects of prepare_to_wait(). We'll
just loop again so this is a benign buglet.
Could we lose a wakeup by running prepaer_to_wait() outside the lock?
I don't think so, but I stopped thinking about it when I saw the above
bug. These two lines should be switched around.
> + ret = (log->w_off == reader->r_off);
> + mutex_unlock(&log->mutex);
> + if (!ret)
> + break;
> +
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> + break;
> + }
> +
> + if (signal_pending(current)) {
> + ret = -EINTR;
> + break;
> + }
> +
> + schedule();
> + }
> +
> + finish_wait(&log->wq, &wait);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&log->mutex);
> +
> + /* is there still something to read or did we race? */
> + if (unlikely(log->w_off == reader->r_off)) {
> + mutex_unlock(&log->mutex);
> + goto start;
> + }
> +
> + /* get the size of the next entry */
> + ret = get_entry_len(log, reader->r_off);
> + if (count < ret) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* get exactly one entry from the log */
> + ret = do_read_log_to_user(log, reader, buf, ret);
> +
> +out:
> + mutex_unlock(&log->mutex);
> +
> + return ret;
> +}
> +
>
> ...
>
> +/*
> + * clock_interval - is a < c < b in mod-space? Put another way, does the line
> + * from a to b cross c?
> + */
This is where my little brain started to hurt. I assume it's correct ;)
> +static inline int clock_interval(size_t a, size_t b, size_t c)
> +{
> + if (b < a) {
> + if (a < c || b >= c)
> + return 1;
> + } else {
> + if (a < c && b >= c)
> + return 1;
> + }
> +
> + return 0;
> +}
The explicit inline(s) are increaseingly old-fashioned. gcc is good
about this now.
>
> ...
>
> +static ssize_t do_write_log_from_user(struct logger_log *log,
> + const void __user *buf, size_t count)
> +{
> + size_t len;
> +
> + len = min(count, log->size - log->w_off);
> + if (len && copy_from_user(log->buffer + log->w_off, buf, len))
> + return -EFAULT;
> +
> + if (count != len)
> + if (copy_from_user(log->buffer, buf + len, count - len))
> + return -EFAULT;
Is it correct to simply return here without updating ->w_off?
We've just copied "len" bytes in from userspace.
> + log->w_off = logger_offset(log->w_off + count);
> +
> + return count;
> +}
> +
> +/*
> + * logger_aio_write - our write method, implementing support for write(),
> + * 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,
> + unsigned long nr_segs, loff_t ppos)
> +{
> + struct logger_log *log = file_get_log(iocb->ki_filp);
> + size_t orig = log->w_off;
> + struct logger_entry header;
> + struct timespec now;
> + ssize_t ret = 0;
> +
> + now = current_kernel_time();
> +
> + header.pid = current->tgid;
> + header.tid = current->pid;
hm, I thought task_struct.pid was the pid.
Also, pids are not unique system-wide. If the user is using PID
namespaces then the logs will contain what may be fatally confusing
duplicates. This needs to be fixed, I expect.
There are broader namespace issues which should be thought about. Does
it make sense for readers in one container to be returned logs from a
different container? Perhaps the do-it-via-a-filesystem proposals can
address this.
> + header.sec = now.tv_sec;
> + header.nsec = now.tv_nsec;
> + header.len = min_t(size_t, iocb->ki_left, LOGGER_ENTRY_MAX_PAYLOAD);
> +
> + /* null writes succeed, return zero */
> + if (unlikely(!header.len))
> + return 0;
> +
> + mutex_lock(&log->mutex);
> +
> + /*
> + * Fix up any readers, pulling them forward to the first readable
> + * entry after (what will be) the new write offset. We do this now
> + * because if we partially fail, we can end up with clobbered log
> + * entries that encroach on readable buffer.
> + */
> + fix_up_readers(log, sizeof(struct logger_entry) + header.len);
> +
> + do_write_log(log, &header, sizeof(struct logger_entry));
> +
> + while (nr_segs-- > 0) {
> + size_t len;
> + ssize_t nr;
> +
> + /* figure out how much of this vector we can keep */
> + len = min_t(size_t, iov->iov_len, header.len - ret);
> +
> + /* write out this segment's payload */
> + nr = do_write_log_from_user(log, iov->iov_base, len);
> + if (unlikely(nr < 0)) {
> + log->w_off = orig;
> + mutex_unlock(&log->mutex);
> + return nr;
> + }
> +
> + iov++;
> + ret += nr;
> + }
> +
> + mutex_unlock(&log->mutex);
> +
> + /* wake up any blocked readers */
> + wake_up_interruptible(&log->wq);
> +
> + return ret;
> +}
> +
>
> ...
>
> +static int logger_release(struct inode *ignored, struct file *file)
> +{
> + if (file->f_mode & FMODE_READ) {
> + struct logger_reader *reader = file->private_data;
> + list_del(&reader->list);
locking for reader->list?
> + kfree(reader);
> + }
> +
> + return 0;
> +}
>
> ...
>
> +static long logger_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
ew...
> +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;
> + return NULL;
> +}
ew...
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Geunsik Lim @ 2012-01-04 15:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Tim Bird, linux-embedded, linux kernel, Arnd Bergmann,
john stultz, Greg KH, Brian Swetland
In-Reply-To: <20111228163925.6c37df26.akpm@linux-foundation.org>
On Thu, Dec 29, 2011 at 9:39 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 21 Dec 2011 14:59:15 -0800
> Tim Bird <tim.bird@am.sony.com> wrote:
>
>> Hi all,
>>
>> I'm looking for feedback on the Android logger code,
>
> The code looks nice.
>
>>
>> ...
>>
>> +/* logger_offset - returns index 'n' into the log via (optimized) modulus */
>> +#define logger_offset(n) ((n) & (log->size - 1))
>> +
>
> This macro depends upon the presence of a local variable called "log"
> and is therefore all stinky. Pass "log" in as an arg and convert it to
> a regular C function.
>
>>
>> ...
>>
>> +/*
>> + * get_entry_len - Grabs the length of the payload of the next entry starting
>> + * from 'off'.
>> + *
>> + * Caller needs to hold log->mutex.
>> + */
>> +static __u32 get_entry_len(struct logger_log *log, size_t off)
>> +{
>> + __u16 val;
>> +
>> + switch (log->size - off) {
>> + case 1:
>> + memcpy(&val, log->buffer + off, 1);
>> + memcpy(((char *) &val) + 1, log->buffer, 1);
>
> So numbers in the buffer are in host endian order. That's worth
> capturing in a comment somewhere.
>
> There must be a way of doing the above more neatly, using
> log->buffer[off] and log->buffer[0]. Perhaps using
> include/linux/unaligned/packed_struct.h.
>
>> + break;
>> + default:
>> + memcpy(&val, log->buffer + off, 2);
>
> get_unaligned()
>
>> + }
>> +
>> + return sizeof(struct logger_entry) + val;
>> +}
>> +
>>
>> ...
>>
>> +static ssize_t logger_read(struct file *file, char __user *buf,
>> + size_t count, loff_t *pos)
>> +{
>> + struct logger_reader *reader = file->private_data;
>> + struct logger_log *log = reader->log;
>> + ssize_t ret;
>> + DEFINE_WAIT(wait);
>> +
>> +start:
>> + while (1) {
>> + prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);
>> +
>> + mutex_lock(&log->mutex);
>
> If mutex_lock() had to wait for the mutex, it will return in state
> TASK_RUNNING, thus rubbing out the effects of prepare_to_wait(). We'll
> just loop again so this is a benign buglet.
>
> Could we lose a wakeup by running prepaer_to_wait() outside the lock?
> I don't think so, but I stopped thinking about it when I saw the above
> bug. These two lines should be switched around.
>
>> + ret = (log->w_off == reader->r_off);
>> + mutex_unlock(&log->mutex);
>> + if (!ret)
>> + break;
>> +
>> + if (file->f_flags & O_NONBLOCK) {
>> + ret = -EAGAIN;
>> + break;
>> + }
>> +
>> + if (signal_pending(current)) {
>> + ret = -EINTR;
>> + break;
>> + }
>> +
>> + schedule();
>> + }
>> +
>> + finish_wait(&log->wq, &wait);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&log->mutex);
>> +
>> + /* is there still something to read or did we race? */
>> + if (unlikely(log->w_off == reader->r_off)) {
>> + mutex_unlock(&log->mutex);
>> + goto start;
>> + }
>> +
>> + /* get the size of the next entry */
>> + ret = get_entry_len(log, reader->r_off);
>> + if (count < ret) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* get exactly one entry from the log */
>> + ret = do_read_log_to_user(log, reader, buf, ret);
>> +
>> +out:
>> + mutex_unlock(&log->mutex);
>> +
>> + return ret;
>> +}
>> +
>>
>> ...
>>
>> +/*
>> + * clock_interval - is a < c < b in mod-space? Put another way, does the line
>> + * from a to b cross c?
>> + */
>
> This is where my little brain started to hurt. I assume it's correct ;)
>
>> +static inline int clock_interval(size_t a, size_t b, size_t c)
>> +{
>> + if (b < a) {
>> + if (a < c || b >= c)
>> + return 1;
>> + } else {
>> + if (a < c && b >= c)
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>
> The explicit inline(s) are increaseingly old-fashioned. gcc is good
> about this now.
>
>>
>> ...
>>
>> +static ssize_t do_write_log_from_user(struct logger_log *log,
>> + const void __user *buf, size_t count)
>> +{
>> + size_t len;
>> +
>> + len = min(count, log->size - log->w_off);
>> + if (len && copy_from_user(log->buffer + log->w_off, buf, len))
>> + return -EFAULT;
>> +
>> + if (count != len)
>> + if (copy_from_user(log->buffer, buf + len, count - len))
>> + return -EFAULT;
>
> Is it correct to simply return here without updating ->w_off?
> We've just copied "len" bytes in from userspace.
>
>> + log->w_off = logger_offset(log->w_off + count);
>> +
>> + return count;
>> +}
>> +
>> +/*
>> + * logger_aio_write - our write method, implementing support for write(),
>> + * 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,
>> + unsigned long nr_segs, loff_t ppos)
>> +{
>> + struct logger_log *log = file_get_log(iocb->ki_filp);
>> + size_t orig = log->w_off;
>> + struct logger_entry header;
>> + struct timespec now;
>> + ssize_t ret = 0;
>> +
>> + now = current_kernel_time();
>> +
>> + header.pid = current->tgid;
>> + header.tid = current->pid;
>
> hm, I thought task_struct.pid was the pid.
It's right.
>
> Also, pids are not unique system-wide. If the user is using PID
I think that this code is correct because current->tgid means
process id of user-space
and current->pid means thread id of user-space actually. Is it not make sense?
> namespaces then the logs will contain what may be fatally confusing duplicates.
> This needs to be fixed, I expect.
header.tid as a thread id of user-space is unique system-wide at least
although header.pid is not unqueue system-wide.
Why should we think/understand that the logs contain duplicated
information to users
who run logcat command for collecting and viewing system debug output?
Thanks.
>
> There are broader namespace issues which should be thought about. Does
> it make sense for readers in one container to be returned logs from a
> different container? Perhaps the do-it-via-a-filesystem proposals can
> address this.
>
>> + header.sec = now.tv_sec;
>> + header.nsec = now.tv_nsec;
>> + header.len = min_t(size_t, iocb->ki_left, LOGGER_ENTRY_MAX_PAYLOAD);
>> +
>> + /* null writes succeed, return zero */
>> + if (unlikely(!header.len))
>> + return 0;
>> +
>> + mutex_lock(&log->mutex);
>> +
>> + /*
>> + * Fix up any readers, pulling them forward to the first readable
>> + * entry after (what will be) the new write offset. We do this now
>> + * because if we partially fail, we can end up with clobbered log
>> + * entries that encroach on readable buffer.
>> + */
>> + fix_up_readers(log, sizeof(struct logger_entry) + header.len);
>> +
>> + do_write_log(log, &header, sizeof(struct logger_entry));
>> +
>> + while (nr_segs-- > 0) {
>> + size_t len;
>> + ssize_t nr;
>> +
>> + /* figure out how much of this vector we can keep */
>> + len = min_t(size_t, iov->iov_len, header.len - ret);
>> +
>> + /* write out this segment's payload */
>> + nr = do_write_log_from_user(log, iov->iov_base, len);
>> + if (unlikely(nr < 0)) {
>> + log->w_off = orig;
>> + mutex_unlock(&log->mutex);
>> + return nr;
>> + }
>> +
>> + iov++;
>> + ret += nr;
>> + }
>> +
>> + mutex_unlock(&log->mutex);
>> +
>> + /* wake up any blocked readers */
>> + wake_up_interruptible(&log->wq);
>> +
>> + return ret;
>> +}
>> +
>>
>> ...
>>
>> +static int logger_release(struct inode *ignored, struct file *file)
>> +{
>> + if (file->f_mode & FMODE_READ) {
>> + struct logger_reader *reader = file->private_data;
>> + list_del(&reader->list);
>
> locking for reader->list?
>
>> + kfree(reader);
>> + }
>> +
>> + return 0;
>> +}
>>
>> ...
>>
>> +static long logger_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
> ew...
>
>> +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;
>> + return NULL;
>> +}
>
> ew...
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Best regards,
Geunsik Lim, Samsung Electronics
Homepage: http://leemgs.fedorapeople.org
-----------------------------------------------------------------------
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-----------------------------------------------------------------------
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Tim Bird @ 2012-01-06 20:56 UTC (permalink / raw)
To: NeilBrown
Cc: Brian Swetland, david@lang.hm, Greg KH, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering, Andrew Morton, david, Paul McKenney,
Alan Stern, Alan Cox, Linus Torvalds
In-Reply-To: <20111222180517.20bce47d@notabene.brown>
I'm back from vacation - sorry for the long delay in responding.
First, let me say thanks to all those who have responded with ideas
and suggestions. I'll be following up on many of them in the new
few weeks. This is a background task for me, so it will likely
go relatively slowly. I have volunteer offers of assistance,
and some resources available for contract work, but it might take time
to set that up. In general, though, things should be able to move forward.
I could respond to a lot of messages individually, but
to conserve space I'll focus on this one, and present an overall
idea for how I plan to proceed.
On 12/21/2011 11:05 PM, NeilBrown wrote:
> Possibly it would be useful to be clear what we all *are* really interested
> in, because I suspect there is a lot of variety.
>
> You appear to be interested in providing a great platform for a phone, in
> minimising unnecessary churn in that platform, and in having the freedom to
> optimise various aspects however you see fit. You appear to have little
> problem with maintaining some components out-of-mainline. This is all
> perfectly valid and very much in the spirit of "freedom" that binds us
> together.
>
> Others want to be able to run a main-line kernel underneath the Android
> user-space - and that is perfectly reasonable as well.
>
> I don't much care about either of those, but I want "Linux" to be "high
> quality" (according to my own personal standards of course) which means
> keeping bad stuff out (though you could argue that the horse has well and
> truly bolted there) and including good and useful stuff in. And I think
> Android has something to offer on both sides there :-)
Thanks very much for this. I think it *is* important to take a step
back and compare goals, to see how we can achieve the best result for
all parties.
>
> Weighing all that up, I don't think it is useful to set our goal on "getting
> Android to use a mainline kernel" - that isn't going to happen.
> Rather we should focus primarily on "making it *possible* to run android
> user-space on mainline".
>
> That could involve two things
> 1/ Making sure the interfaces that Android uses are abstracted at a suitable
> level so that when running a mainline kernel you can just slip in an
> alternate shared library with compatible functionality.
> 2/ Adding functionality to Linux so that it is possible to provide the
> functionality that Android needs.
Agreed.
> Android should not *have* to use the interface that the "mainline
> community" decides is preferred, but nor should mainline be required to
> include the drivers that Android wants to use. History shows us that isn't
> going to happen.
I agree with the sentiment in the first sentence, but I would like to
make a few observations about this code, and the problem in general,
for consideration. I'll do this below because it's a bit philosophical,
and I'd rather focus on "the way forward" first.
> But if there was a fairly low-level API that Android used, then those in the
> community could who want Android on a mainline kernel could work to implement
> that API with whatever mixture of user-space and kernel-space that achieves
> consensus.
> Android could of course change to use the community version eventually if
> they found that was a good thing, or could keep using their own.
>
> So: bringing this back to the android logger...
> What I would like to see is a low-level API which is used to access logging
> for which alternate implementations could be written. Ideally this would
> be embodied in a single .so, but we have the source so that doesn't need to
> be a barrier.
> Then we could argue to our heart's content about how best to implement that
> API - Journal and nsyslogd and rsyslogd could all implement it in different
> ways and we could be one step closer to running Android on a mainline
> kernel, but the Android developers don't need to be involved (but can if
> they want to of course).
>
> I would be important that the API is specified clearly - neither under
> specified nor over-specified. That means that the Android implementation
> would need to explicitly forbid anything that isn't explicit permitted.
> This is because most testing will happen on the Android platform so it's
> actual behaviour will become the defacto standard.
>
> Could that be a possible way forward?
I'd like to pursue this, as well as some of the minor code cleanups
suggested by Andrew Morton. Here's what I'm thinking:
I'd like to implement Neil's file-system based solution as a test case,
and compare that with the existing code. I like the elegance of using
existing filesystem semantics, and the removal of some hard-coded limits
and policy from the kernel. A lower priority would be to also try
a user-space-only solution, as described by Arnd. Optimally, it would
be nice to have all three systems to compare (char dev, log fs, and
ram fs) against each other.
I'll try to use these under the existing logger library API to see if the
current semantics expected by Android user space can be preserved. This
would minimize the churn to Android user space. Before this (or any other
changes for that matter) could be rolled out in an official Android
release, there would be a lot of testing required, to make sure something
hasn't been broken. I want to avoid asking the Google developers to
do this, as they have something that works now, and there's little incentive
to change it. I'm tackling this problem as something of a wider issue
for the industry.
Separately, I'd like to apply the changes requested by Andrew Morton
to the existing char dev driver, as just simple cleanups. This, however,
also will require some testing to avoid regressions.
On a separate track, I want to compare the requirements discussed in this
thread with what systemd is doing (and how /dev/kmsg is being used) to see
if there are issues that inform a possible more general solution for
logging in the future. To this end, it might be good to set up a
meeting at a future event (ELC or collab summit, depending on what
people plan to come to) to discuss things face-to-face.
I'm hoping that the existing code can live in staging while we sort
this out longer term. This code has lived out of mainline for several
years, and I don't think we're in any kind of rush. I plan to record
the requirements and the plan for this on a wiki page I've set up at:
http://elinux.org/Mainline_Android_logger_project, but I may also record
some of the simple cleanup requests in a TODO file in staging.
Also, separately, we'll likely do some benchmarking of the various
systems, as part of our overall comparison effort - to address questions
about the tradeoffs involved - particularly between implementing this
in or out of kernel and between existing and new kernel infrastructure.
[warning - next part is long...]
Now, having said all that, let me go off into some philosophical weeds...
This code is only about 700 lines long, and specializes the kernel for
an extremely popular (by usage) user space stack. Code of
lower quality which specializes the kernel for much less-used hardware
has been admitted into the kernel with significantly less fuss than
this code has received. That's not an argument to accept the code
as is, it's just an observation about the relative hurdle that this
code faces compared to lots of other code that goes into the kernel.
(And please don't interpret this as dissatisfaction with the feedback
received.)
If this code were a character driver for an obscure serial port
on a lesser-known chip architecture, I don't think it would get
any attention at all. As it is, it's looking like at least a few
man months of work will be required, as well as some relatively
unneeded changes to Android user space, to get this feature into
a permanently acceptable state. I wouldn't be surprised to see
this stretch into a few calendar years.
Code that specializes the kernel in weird ways is accepted into
the kernel all the time, and I've tried to figure out why this
particular bit of code is treated differently. Especially since
this code is self-contained, configurable, and imposes no
perceivable long-term maintenance burden. (That's not true of
all Android patches, but I believe it's true of this one).
I have a few theories:
1) this is not tied to hardware, and as such represents a general
feature (but people are not at all required to treat it as such,
just as they are not required to use other people's weird drivers).
2) people want to avoid duplication with other similar features
(again, since it's self-contained and configurable, I don't know
why it would bother people if this existed in tandem with other
solutions - especially since it's so small)
3) there is really no maintainer for this feature category, so
discussions get bogged down as varying requirements and solutions
are suggested, which can not easily be compared against each other
(especially for non-existent implementations) In particular, it's
unclear who I have to get the approval of for this code or some
derivative of it to be accepted. That makes the development task
a very open-ended one.
4) this is for a popular use case, as opposed to some minor
outlying thing, and so people perceive the need to get it
exactly right. In this sense, the code would be a victim of
it's own success.
5) blocking this is perceived to be a way to accomplish a
larger, related goal (if this is true then it has lots of
interesting implications for the economics of open source
work)
In general, there is a tension between the normal nature of adapting
the kernel to the most general use cases, and the specialization
that is performed in developing an embedded product. Often
times, solutions to embedded requirements are very finely tuned
to a particular field of use or situation, and don't lend themselves
easily to the type of generalization that mainlining usually requires.
Which brings me to my last point and a question.
Is it inconceivable for there to be a category of code in the
Linux kernel which supports ONLY Android user space, and no
other? That is, must every Android patch be generalized in
some manner to a broader use case?
I suspect some of them (particularly binder) will not lend
themselves easily to this type of generalization.
Knowing the answer to this question would help me gauge the
amount of effort required for this project, and the
net value of continuing it.
Thanks and regards,
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Greg KH @ 2012-01-06 21:20 UTC (permalink / raw)
To: Tim Bird
Cc: NeilBrown, Brian Swetland, david@lang.hm, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering, Andrew Morton, Paul McKenney, Alan Stern,
Alan Cox, Linus Torvalds
In-Reply-To: <4F075FFB.4090703@am.sony.com>
On Fri, Jan 06, 2012 at 12:56:27PM -0800, Tim Bird wrote:
> Now, having said all that, let me go off into some philosophical weeds...
Ah, this should be fun :)
> This code is only about 700 lines long, and specializes the kernel for
> an extremely popular (by usage) user space stack. Code of
> lower quality which specializes the kernel for much less-used hardware
> has been admitted into the kernel with significantly less fuss than
> this code has received. That's not an argument to accept the code
> as is, it's just an observation about the relative hurdle that this
> code faces compared to lots of other code that goes into the kernel.
> (And please don't interpret this as dissatisfaction with the feedback
> received.)
>
> If this code were a character driver for an obscure serial port
> on a lesser-known chip architecture, I don't think it would get
> any attention at all. As it is, it's looking like at least a few
> man months of work will be required, as well as some relatively
> unneeded changes to Android user space, to get this feature into
> a permanently acceptable state. I wouldn't be surprised to see
> this stretch into a few calendar years.
>
> Code that specializes the kernel in weird ways is accepted into
> the kernel all the time, and I've tried to figure out why this
> particular bit of code is treated differently. Especially since
> this code is self-contained, configurable, and imposes no
> perceivable long-term maintenance burden. (That's not true of
> all Android patches, but I believe it's true of this one).
>
> I have a few theories:
> 1) this is not tied to hardware, and as such represents a general
> feature (but people are not at all required to treat it as such,
> just as they are not required to use other people's weird drivers).
That is exactly true.
> 2) people want to avoid duplication with other similar features
> (again, since it's self-contained and configurable, I don't know
> why it would bother people if this existed in tandem with other
> solutions - especially since it's so small)
Again, very true in the duplication sense.
It is general code, not tied to hardware, so it better justify itself
somehow.
> 3) there is really no maintainer for this feature category, so
> discussions get bogged down as varying requirements and solutions
> are suggested, which can not easily be compared against each other
> (especially for non-existent implementations) In particular, it's
> unclear who I have to get the approval of for this code or some
> derivative of it to be accepted. That makes the development task
> a very open-ended one.
No, I don't think this is a problem, you have bigger ones to deal with
:)
> 4) this is for a popular use case, as opposed to some minor
> outlying thing, and so people perceive the need to get it
> exactly right. In this sense, the code would be a victim of
> it's own success.
It's not the code's "success" here, it's the "this solves a general
problem in a very specific way" issue, that just happens to be a use
case a lot of different people want to see addressed. And since it is
only solving it in a narrow way, that's a problem for all of those other
people.
> 5) blocking this is perceived to be a way to accomplish a
> larger, related goal (if this is true then it has lots of
> interesting implications for the economics of open source
> work)
No, I don't think that's the issue here.
> In general, there is a tension between the normal nature of adapting
> the kernel to the most general use cases, and the specialization
> that is performed in developing an embedded product. Often
> times, solutions to embedded requirements are very finely tuned
> to a particular field of use or situation, and don't lend themselves
> easily to the type of generalization that mainlining usually requires.
Why do people get hung up on the "embedded is special" case here. Fact,
it's not. Do you somehow think that the work done for the HUGE
multiprocessor machines 10 years ago would have gotten away with hacks
saying, "this is a special case, only we care about this", when they
were dealing with 4 and 8 way machines? No, they didn't, and as such,
there's nothing special with embedded here either. Matter of fact, your
machines are now more powerful than those old 10 year old machines, and
are really general purpose computers, just used in a single/limited
manner, again, just like those original big SMP machines were.
So, I've said it many times before, and I'll say it again:
Yes, you are special and unique, just like everyone else.
The next person who says the "embedded is different" phrase again, owes
me a beer of my choice.
> Which brings me to my last point and a question.
> Is it inconceivable for there to be a category of code in the
> Linux kernel which supports ONLY Android user space, and no
> other? That is, must every Android patch be generalized in
> some manner to a broader use case?
No, that's fine, I have no problem with a drivers/android/ directory for
android only stuff, because:
> I suspect some of them (particularly binder) will not lend
> themselves easily to this type of generalization.
Exactly, that is where the binder code should eventually end up, and
probably a few other minor bits. But for almost everything else (and
really, there isn't all that much android-only code we are talking about
here:
$ wc drivers/staging/android/* drivers/staging/android/switch/* -l | tail -n 1
8429 total
And the switch code is already being worked on to be generalized by
others, so that means we only have:
$ wc drivers/staging/android/* -l | tail -n 1
8015 total
to worry about here.
thanks,
greg k-h
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Tim Bird @ 2012-01-06 22:41 UTC (permalink / raw)
To: Greg KH
Cc: NeilBrown, Brian Swetland, david@lang.hm, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering, Andrew Morton, Paul McKenney, Alan Stern,
Alan Cox, Linus Torvalds
In-Reply-To: <20120106212054.GA19509@suse.de>
On 01/06/2012 01:20 PM, Greg KH wrote:
> On Fri, Jan 06, 2012 at 12:56:27PM -0800, Tim Bird wrote:
>> 4) this is for a popular use case, as opposed to some minor
>> outlying thing, and so people perceive the need to get it
>> exactly right. In this sense, the code would be a victim of
>> it's own success.
>
> It's not the code's "success" here, it's the "this solves a general
> problem in a very specific way" issue, that just happens to be a use
> case a lot of different people want to see addressed. And since it is
> only solving it in a narrow way, that's a problem for all of those other
> people.
It's only a problem in the sense that those different people
don't get something for free. The presence of an isolated,
configurable driver somewhere in the kernel source, that
addresses one group's issues and not another's doesn't impose
any measurable burden on that other group.
It's more of an opportunity cost thing. The opportunity
to develop something more general can be reduced when a narrow
solution is accepted. But this is exactly related to
encouraging people to develop more general solutions, when
narrow ones accomplish what they need (see point 5).
I understand that the whole model is built on people contributing
code that not only addresses their own needs, but the needs of
others. But of course there should be balance. For example
I have concerns about integrating this into the printk code path,
because I think that code path is complex enough as it is,
and that combining this functionality with that is likely to
create more maintenance problems rather than less in the
long run. (But I'll still plan to look at this option ...)
Sometimes, features should just remain separate.
>
>> 5) blocking this is perceived to be a way to accomplish a
>> larger, related goal (if this is true then it has lots of
>> interesting implications for the economics of open source
>> work)
>
> No, I don't think that's the issue here.
I don't think anyone is thinking: "If I block this
I can get Tim to do x". But there *is* a desire to
create a more general logging solution. And not
accepting this as is, is a way to encourage people to
work on that.
For some situations, I would object to that. But in
this case I actually agree that there are more potential users
of this, and it would be nice to solve more problems than
just the Android case, with a consolidated code base. That's
why I'm willing to invest in researching other loggers and
doing some experimental development.
>
>> In general, there is a tension between the normal nature of adapting
>> the kernel to the most general use cases, and the specialization
>> that is performed in developing an embedded product. Often
>> times, solutions to embedded requirements are very finely tuned
>> to a particular field of use or situation, and don't lend themselves
>> easily to the type of generalization that mainlining usually requires.
I should have left off "embedded product" for this thread. There is
a tension in general between generalization and specialization. And
that tension shows up in a lot of discussions about mainlining code.
My goal here is to determine where people think the balance lies
for this particular code.
> Why do people get hung up on the "embedded is special" case here. Fact,
> it's not. Do you somehow think that the work done for the HUGE
> multiprocessor machines 10 years ago would have gotten away with hacks
> saying, "this is a special case, only we care about this", when they
> were dealing with 4 and 8 way machines? No, they didn't, and as such,
> there's nothing special with embedded here either. Matter of fact, your
> machines are now more powerful than those old 10 year old machines, and
> are really general purpose computers, just used in a single/limited
> manner, again, just like those original big SMP machines were.
>
> So, I've said it many times before, and I'll say it again:
>
> Yes, you are special and unique, just like everyone else.
>
> The next person who says the "embedded is different" phrase again, owes
> me a beer of my choice.
Consider this your free beer token... :-)
I believe "embedded is different" because we throw away so much code,
that I just don't think desktop and enterprise do. The number of
non-mainlined BSPs that have gone straight from product release to
bit bucket is truly staggering.
You're only looking at the Android case, where the machines are as
powerful as they were 10 years ago. Broad swaths of embedded
are not on that kind of hardware. It's different (there it is again,
is that two beers?) being at the bottom side of the scalability
spectrum rather than at the top. Everyone expected that eventually
there would be more SMP machines in the world. This is not true of
all embedded processors. We're used to developing code that we'll
throw away after a single release.
But I think this is beside the point for this particular code.
I agree that it's worth some effort to try to generalize this
logger code. Indeed, when I first saw the logger code I thought,
"this would be nice to use on my other projects". So we'll spend
some time trying to see if it can address more than a single group's
requirements, without gumming up the features that make it valuable
to Android as is. I'll tell you in a few months whether I think
it's practical or worth it.
>> Which brings me to my last point and a question.
>> Is it inconceivable for there to be a category of code in the
>> Linux kernel which supports ONLY Android user space, and no
>> other? That is, must every Android patch be generalized in
>> some manner to a broader use case?
>
> No, that's fine, I have no problem with a drivers/android/ directory for
> android only stuff, because:
>
>> I suspect some of them (particularly binder) will not lend
>> themselves easily to this type of generalization.
>
> Exactly, that is where the binder code should eventually end up, and
> probably a few other minor bits.
That's good to hear.
> But for almost everything else (and
> really, there isn't all that much android-only code we are talking about
> here:
> $ wc drivers/staging/android/* drivers/staging/android/switch/* -l | tail -n 1
> 8429 total
>
> And the switch code is already being worked on to be generalized by
> others, so that means we only have:
> $ wc drivers/staging/android/* -l | tail -n 1
> 8015 total
> to worry about here.
Well, I'm not sure you've gotten it all (my count is closer to 20k,
some of it sprinkled in a few weird places), but I agree completely
that it's not a lot of code, which is encouraging. Did you put
anything in besides what's in staging?
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Greg KH @ 2012-01-06 23:17 UTC (permalink / raw)
To: Tim Bird
Cc: NeilBrown, Brian Swetland, david@lang.hm, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering, Andrew Morton, Paul McKenney, Alan Stern,
Alan Cox, Linus Torvalds
In-Reply-To: <4F0778B0.3050104@am.sony.com>
On Fri, Jan 06, 2012 at 02:41:52PM -0800, Tim Bird wrote:
> > But for almost everything else (and
> > really, there isn't all that much android-only code we are talking about
> > here:
> > $ wc drivers/staging/android/* drivers/staging/android/switch/* -l | tail -n 1
> > 8429 total
> >
> > And the switch code is already being worked on to be generalized by
> > others, so that means we only have:
> > $ wc drivers/staging/android/* -l | tail -n 1
> > 8015 total
> > to worry about here.
>
> Well, I'm not sure you've gotten it all (my count is closer to 20k,
> some of it sprinkled in a few weird places), but I agree completely
> that it's not a lot of code, which is encouraging. Did you put
> anything in besides what's in staging?
Yes, it should all be under drivers/staging/android in linux-next, soon
to go to Linus.
What do you think I have missed? I only know of two missing bits:
- android USB gadget drivers/stuff
- wakelocks
The first is being addressed by the Google developers, see the post to
the linux-usb mailing list the end of last month with the code, and all
of the review comments on it.
The second is being worked on by someone else, as I'm sure you are
already aware.
Other than that, I don't see anything in the common.git android tree
that I am missing, other than some odd driver updates which should all
be upstream already from what I can tell.
So how are our line counts so far off?
thanks,
greg k-h
^ permalink raw reply
* Re: RFC: android logger feedback request
From: Greg KH @ 2012-01-06 23:35 UTC (permalink / raw)
To: Tim Bird
Cc: NeilBrown, Brian Swetland, david@lang.hm, linux-embedded,
linux kernel, Arnd Bergmann, john stultz, Kay Sievers,
Lennart Poettering, Andrew Morton, Paul McKenney, Alan Stern,
Alan Cox, Linus Torvalds
In-Reply-To: <4F0778B0.3050104@am.sony.com>
On Fri, Jan 06, 2012 at 02:41:52PM -0800, Tim Bird wrote:
> On 01/06/2012 01:20 PM, Greg KH wrote:
> > On Fri, Jan 06, 2012 at 12:56:27PM -0800, Tim Bird wrote:
> >> 4) this is for a popular use case, as opposed to some minor
> >> outlying thing, and so people perceive the need to get it
> >> exactly right. In this sense, the code would be a victim of
> >> it's own success.
> >
> > It's not the code's "success" here, it's the "this solves a general
> > problem in a very specific way" issue, that just happens to be a use
> > case a lot of different people want to see addressed. And since it is
> > only solving it in a narrow way, that's a problem for all of those other
> > people.
>
> It's only a problem in the sense that those different people
> don't get something for free. The presence of an isolated,
> configurable driver somewhere in the kernel source, that
> addresses one group's issues and not another's doesn't impose
> any measurable burden on that other group.
That's been proven incorrect over the long-run as we all must now
maintain that user/kernel API for forever.
You could say the same thing for adding new ioctls for individual
drivers instead of creating a new syscall. Sometimes, in rare cases, it
does make sense to create a new ioctl. But that decision has to be
taken very carefully, as again, it needs to be standardized with all
other drivers of that time, and maintained for forever.
> It's more of an opportunity cost thing. The opportunity
> to develop something more general can be reduced when a narrow
> solution is accepted. But this is exactly related to
> encouraging people to develop more general solutions, when
> narrow ones accomplish what they need (see point 5).
When people work together on their narrow goals, they end up creating
something that works for way more than just themselves, and in fact, it
turns out it works better for people who originally wasn't even aware of
the issues involved.
Again, this has been proven over time with the Linux kernel development
process. I wrote a whole chapter on this very topic in the book,
"Beautiful Code", if you want to hear more about it.
In fact, I would argue, that is one of the top reasons why Linux has
succeeded so well. Again, you really aren't unique, others will have
the same issues/problems that you do, and if you solve them in a case
that works for more than one, it makes everything better.
> I understand that the whole model is built on people contributing
> code that not only addresses their own needs, but the needs of
> others. But of course there should be balance. For example
> I have concerns about integrating this into the printk code path,
> because I think that code path is complex enough as it is,
> and that combining this functionality with that is likely to
> create more maintenance problems rather than less in the
> long run. (But I'll still plan to look at this option ...)
Complex code paths are tough, I will grant you that. But if it was
easy, then, you wouldn't be the person for the job :)
> Sometimes, features should just remain separate.
When they do different things, of course. When they do the same thing,
of course not.
> > Why do people get hung up on the "embedded is special" case here. Fact,
> > it's not. Do you somehow think that the work done for the HUGE
> > multiprocessor machines 10 years ago would have gotten away with hacks
> > saying, "this is a special case, only we care about this", when they
> > were dealing with 4 and 8 way machines? No, they didn't, and as such,
> > there's nothing special with embedded here either. Matter of fact, your
> > machines are now more powerful than those old 10 year old machines, and
> > are really general purpose computers, just used in a single/limited
> > manner, again, just like those original big SMP machines were.
> >
> > So, I've said it many times before, and I'll say it again:
> >
> > Yes, you are special and unique, just like everyone else.
> >
> > The next person who says the "embedded is different" phrase again, owes
> > me a beer of my choice.
>
> Consider this your free beer token... :-)
Oooh, nice, I'll take you up on that at ELC in a few weeks :)
> I believe "embedded is different" because we throw away so much code,
> that I just don't think desktop and enterprise do. The number of
> non-mainlined BSPs that have gone straight from product release to
> bit bucket is truly staggering.
Yes, and that's sad. And maybe, if embedded feels that they are so
special, and that contributing to mainline doesn't help or matter, then
they shouldn't.
Yes, I mean it, why push stuff upstream if you aren't going to maintain
it and no one will ever build on top of it.
Oh wait, you will end up building on it with your next iteration, so if
you throw it away, oops, you just wasted time and money.
And again, this mirrors the super computer case EXACTLY! In fact, I
would argue that there are less machines shipped for those types of
systems, than embedded, and do you see those developers saying that they
should just keep their code out of mainline? Or do they actually
realize that it is cheaper, in time and money to mainline stuff.
Remember, Intel and IBM has said publically, numerous times, that it
makes business and money sense to do this. Are the people at these
companies somehow wrong? Do they not also support the "embedded"
markets?
> You're only looking at the Android case, where the machines are as
> powerful as they were 10 years ago. Broad swaths of embedded
> are not on that kind of hardware. It's different (there it is again,
> is that two beers?) being at the bottom side of the scalability
> spectrum rather than at the top. Everyone expected that eventually
> there would be more SMP machines in the world. This is not true of
> all embedded processors. We're used to developing code that we'll
> throw away after a single release.
Again, same goes for super computers.
And again, if it really is going to be thrown away, then by all means,
throw it away. But I will bet you the beer you owe me, odds are you
want that code again in the future to build on top of.
Unless it's an obsolete architecture like Voyager or something, then
just throw that away, no one would ever build on top of that :)
Oh wait, again, look at the benifits Linux overall has achieved from
supporting something "odd" and "obsolete" in the mainline kernel, to
help abstract core functionality to make things more flexable and
workable for {gasp} even the embedded folks.
Sorry, again, I don't buy your argument one bit.
> But I think this is beside the point for this particular code.
I agree.
thanks,
greg "beer's on Tim!" k-h
^ permalink raw reply
* [PATCH 1/5] logger: Change logger_offset() from macro to function
From: Tim Bird @ 2012-02-08 2:21 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
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 <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index ffc2d04..92456d7 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -60,7 +60,11 @@ struct logger_reader {
};
/* logger_offset - returns index 'n' into the log via (optimized) modulus */
-#define logger_offset(n) ((n) & (log->size - 1))
+size_t logger_offset(struct logger_log *log, size_t n)
+{
+ return n & (log->size-1);
+}
+
/*
* 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
^ permalink raw reply related
* [PATCH 2/5] logger: simplify and optimize get_entry_len
From: Tim Bird @ 2012-02-08 2:28 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
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.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..92cfd94 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -93,19 +93,23 @@ 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'.
*
+ * 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;
- switch (log->size - off) {
- case 1:
- memcpy(&val, log->buffer + off, 1);
- memcpy(((char *) &val) + 1, log->buffer, 1);
- break;
- default:
- memcpy(&val, log->buffer + off, 2);
+ if (unlikely(log->size - off == 1)) {
+ /* at end of buffer, handle wrap */
+ ((unsigned char *)&val)[0] = log->buffer[off];
+ ((unsigned char *)&val)[1] = log->buffer[0];
+ } else {
+ ((unsigned char *)&val)[0] = log->buffer[off];
+ ((unsigned char *)&val)[1] = log->buffer[off+1];
}
return sizeof(struct logger_entry) + val;
--
1.7.2.3
^ permalink raw reply related
* [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock
From: Tim Bird @ 2012-02-08 2:30 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
If mutex_lock waits, it will return in state TASK_RUNNING,
rubbing out the effect of prepare_to_wait().
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92cfd94..54b7cdf 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -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
^ permalink raw reply related
* [PATCH 4/5] logger: clarify code in clock_interval
From: Tim Bird @ 2012-02-08 2:32 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
Add commentary, rename the function and make the code easier to read.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 28 ++++++++++++++++++++--------
1 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 54b7cdf..8d9d4f1 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -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
^ permalink raw reply related
* [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user
From: Tim Bird @ 2012-02-08 2:34 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
Add comment to explain when w_off is not updated in case of failed second
fragment copy to buffer.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 8d9d4f1..115d8ed 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -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
^ permalink raw reply related
* Re: [PATCH 1/5] logger: Change logger_offset() from macro to function
From: Frank Rowand @ 2012-02-08 3:22 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
On 02/07/12 18:21, Tim Bird wrote:
> 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 <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
> };
>
> /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n) ((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> + return n & (log->size-1);
return n & (log->size - 1);
> +}
> +
Reviewed-by: Frank Rowand <frank.rowand@am.sony.com>
^ permalink raw reply
* [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
From: Tim Bird @ 2012-02-08 18:37 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland, Dima Zavin,
Andrew Morton
In-Reply-To: <4F31DDC7.1050107@am.sony.com>
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 <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..3475cb7 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -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'.
*
+ * 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;
- switch (log->size - off) {
- case 1:
- memcpy(&val, log->buffer + off, 1);
- memcpy(((char *) &val) + 1, log->buffer, 1);
- 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];
+ else
+ ((__u8 *)&val)[1] = log->buffer[0];
return sizeof(struct logger_entry) + val;
}
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH 1/5] logger: Change logger_offset() from macro to function
From: Dima Zavin @ 2012-02-09 4:54 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Andrew Morton
In-Reply-To: <4F31DC31.6040303@am.sony.com>
small style nit otherwise looks ok.
On Tue, Feb 7, 2012 at 6:21 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> 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 <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
> };
>
> /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n) ((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> + return n & (log->size-1);
spaces around -
> +}
> +
>
> /*
> * 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
>
^ permalink raw reply
* Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
From: Dima Zavin @ 2012-02-09 5:15 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Andrew Morton
In-Reply-To: <4F32C105.4060700@am.sony.com>
On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird <tim.bird@am.sony.com> wrote:
> 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 <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 20 ++++++++++++--------
> 1 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92456d7..3475cb7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -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'.
> *
> + * 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;
>
> - switch (log->size - off) {
> - case 1:
> - memcpy(&val, log->buffer + off, 1);
> - memcpy(((char *) &val) + 1, log->buffer, 1);
> - 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.
> + else
> + ((__u8 *)&val)[1] = log->buffer[0];
>
> return sizeof(struct logger_entry) + val;
> }
> --
> 1.7.2.3
>
^ permalink raw reply
* Re: [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock
From: Dima Zavin @ 2012-02-09 5:56 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Andrew Morton
In-Reply-To: <4F31DE31.3040001@am.sony.com>
On Tue, Feb 7, 2012 at 6:30 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> If mutex_lock waits, it will return in state TASK_RUNNING,
> rubbing out the effect of prepare_to_wait().
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
Acked-by: Dima Zavin <dima@android.com>
> ---
> drivers/staging/android/logger.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92cfd94..54b7cdf 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -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
>
^ permalink raw reply
* Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
From: Tim Bird @ 2012-02-09 5:58 UTC (permalink / raw)
To: Dima Zavin
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Andrew Morton
In-Reply-To: <CAPz4a6CFd_L=hDw9uG56ZnhiqqnsPCLTR_3G=Zo5KDnTP=MwiA@mail.gmail.com>
On 2/8/2012 9:15 PM, Dima Zavin wrote:
> On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird<tim.bird@am.sony.com> wrote:
>> 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<tim.bird@am.sony.com>
>> ---
>> drivers/staging/android/logger.c | 20 ++++++++++++--------
>> 1 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
>> index 92456d7..3475cb7 100644
>> --- a/drivers/staging/android/logger.c
>> +++ b/drivers/staging/android/logger.c
>> @@ -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'.
>> *
>> + * 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;
>
That's a good idea. I was looking for a way to use a byte array that
wouldn't get misaligned if the function declaration changed. I'll
test this out and see what it looks like.
>> - switch (log->size - off) {
>> - case 1:
>> - memcpy(&val, log->buffer + off, 1);
>> - memcpy(((char *)&val) + 1, log->buffer, 1);
>> - 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.
Yeah. I keep omitting spaces. I'll fix this, and the ones
mentioned on patch 1/5.
-- Tim
^ permalink raw reply
* Re: [PATCH 4/5] logger: clarify code in clock_interval
From: Dima Zavin @ 2012-02-09 6:09 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Andrew Morton
In-Reply-To: <4F31DEA2.7000702@am.sony.com>
To be honest, i don't find the new logic code much different or
cleaner than the old code. I think just documenting (as well as the
function rename) as you've done without inverting the logic would be
sufficient. The ascii art helps immensely.
--Dima
On Tue, Feb 7, 2012 at 6:32 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Add commentary, rename the function and make the code easier to read.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 28 ++++++++++++++++++++--------
> 1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 54b7cdf..8d9d4f1 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -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
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox