Changelog
Fixes comments by akpm
- unnecessary initialization of delayacct_on
- use kmem_cache_zalloc
- redundant check in __delayacct_tsk_exit
Other changes
- do not mix tabs and spaces
delayacct-setup.patch
Initialization code related to collection of per-task "delay"
statistics which measure how long it had to wait for cpu,
sync block io, swapping etc. The collection of statistics and
the interface are in other patches. This patch sets up the data
structures and allows the statistics collection to be disabled
through a kernel boot paramater.
Signed-off-by: Shailabh Nagar <[email protected]>
Signed-off-by: Balbir Singh <[email protected]>
---
Documentation/kernel-parameters.txt | 2
include/linux/delayacct.h | 69 ++++++++++++++++++++++++++++
include/linux/sched.h | 20 ++++++++
include/linux/time.h | 10 ++++
init/Kconfig | 10 ++++
init/main.c | 2
kernel/Makefile | 1
kernel/delayacct.c | 87 ++++++++++++++++++++++++++++++++++++
kernel/exit.c | 2
kernel/fork.c | 2
10 files changed, 205 insertions(+)
diff -puN Documentation/kernel-parameters.txt~delayacct-setup Documentation/kernel-parameters.txt
--- linux-2.6.17-rc3/Documentation/kernel-parameters.txt~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/Documentation/kernel-parameters.txt 2006-04-28 23:47:55.000000000 +0530
@@ -430,6 +430,8 @@ running once the system is up.
Format: <area>[,<node>]
See also Documentation/networking/decnet.txt.
+ delayacct [KNL] Enable per-task delay accounting
+
devfs= [DEVFS]
See Documentation/filesystems/devfs/boot-options.
diff -puN /dev/null include/linux/delayacct.h
--- /dev/null 2004-06-24 23:34:38.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/delayacct.h 2006-05-02 09:44:48.000000000 +0530
@@ -0,0 +1,69 @@
+/* delayacct.h - per-task delay accounting
+ *
+ * Copyright (C) Shailabh Nagar, IBM Corp. 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_DELAYACCT_H
+#define _LINUX_DELAYACCT_H
+
+#include <linux/sched.h>
+
+#ifdef CONFIG_TASK_DELAY_ACCT
+
+extern int delayacct_on; /* Delay accounting turned on/off */
+extern kmem_cache_t *delayacct_cache;
+extern void delayacct_init(void);
+extern void __delayacct_tsk_init(struct task_struct *);
+extern void __delayacct_tsk_exit(struct task_struct *);
+
+static inline void delayacct_set_flag(int flag)
+{
+ if (current->delays)
+ current->delays->flags |= flag;
+}
+
+static inline void delayacct_clear_flag(int flag)
+{
+ if (current->delays)
+ current->delays->flags &= ~flag;
+}
+
+static inline void delayacct_tsk_init(struct task_struct *tsk)
+{
+ /* reinitialize in case parent's non-null pointer was dup'ed*/
+ tsk->delays = NULL;
+ if (unlikely(delayacct_on))
+ __delayacct_tsk_init(tsk);
+}
+
+static inline void delayacct_tsk_exit(struct task_struct *tsk)
+{
+ if (tsk->delays)
+ __delayacct_tsk_exit(tsk);
+}
+
+#else
+static inline void delayacct_set_flag(int flag)
+{}
+static inline void delayacct_clear_flag(int flag)
+{}
+static inline void delayacct_init(void)
+{}
+static inline void delayacct_tsk_init(struct task_struct *tsk)
+{}
+static inline void delayacct_tsk_exit(struct task_struct *tsk)
+{}
+#endif /* CONFIG_TASK_DELAY_ACCT */
+
+#endif
diff -puN include/linux/sched.h~delayacct-setup include/linux/sched.h
--- linux-2.6.17-rc3/include/linux/sched.h~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/sched.h 2006-05-02 09:44:48.000000000 +0530
@@ -536,6 +536,23 @@ struct sched_info {
extern struct file_operations proc_schedstat_operations;
#endif
+#ifdef CONFIG_TASK_DELAY_ACCT
+struct task_delay_info {
+ spinlock_t lock;
+ unsigned int flags; /* Private per-task flags */
+
+ /* For each stat XXX, add following, aligned appropriately
+ *
+ * struct timespec XXX_start, XXX_end;
+ * u64 XXX_delay;
+ * u32 XXX_count;
+ *
+ * Atomicity of updates to XXX_delay, XXX_count protected by
+ * single lock above (split into XXX_lock if contention is an issue).
+ */
+};
+#endif
+
enum idle_type
{
SCHED_IDLE,
@@ -888,6 +905,9 @@ struct task_struct {
* cache last used pipe for splice
*/
struct pipe_inode_info *splice_pipe;
+#ifdef CONFIG_TASK_DELAY_ACCT
+ struct task_delay_info *delays;
+#endif
};
static inline pid_t process_group(struct task_struct *tsk)
diff -puN include/linux/time.h~delayacct-setup include/linux/time.h
--- linux-2.6.17-rc3/include/linux/time.h~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/time.h 2006-04-28 23:47:55.000000000 +0530
@@ -68,6 +68,16 @@ extern unsigned long mktime(const unsign
extern void set_normalized_timespec(struct timespec *ts, time_t sec, long nsec);
/*
+ * sub = end - start, in normalized form
+ */
+static inline void timespec_sub(struct timespec *start, struct timespec *end,
+ struct timespec *sub)
+{
+ set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
+ end->tv_nsec - start->tv_nsec);
+}
+
+/*
* Returns true if the timespec is norm, false if denorm:
*/
#define timespec_valid(ts) \
diff -puN init/Kconfig~delayacct-setup init/Kconfig
--- linux-2.6.17-rc3/init/Kconfig~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/init/Kconfig 2006-05-02 09:44:40.000000000 +0530
@@ -150,6 +150,16 @@ config BSD_PROCESS_ACCT_V3
for processing it. A preliminary version of these tools is available
at <http://www.physik3.uni-rostock.de/tim/kernel/utils/acct/>.
+config TASK_DELAY_ACCT
+ bool "Enable per-task delay accounting (EXPERIMENTAL)"
+ help
+ Collect information on time spent by a task waiting for system
+ resources like cpu, synchronous block I/O completion and swapping
+ in pages. Such statistics can help in setting a task's priorities
+ relative to other tasks for cpu, io, rss limits etc.
+
+ Say N if unsure.
+
config SYSCTL
bool "Sysctl support"
---help---
diff -puN init/main.c~delayacct-setup init/main.c
--- linux-2.6.17-rc3/init/main.c~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/init/main.c 2006-05-02 09:44:21.000000000 +0530
@@ -47,6 +47,7 @@
#include <linux/rmap.h>
#include <linux/mempolicy.h>
#include <linux/key.h>
+#include <linux/delayacct.h>
#include <asm/io.h>
#include <asm/bugs.h>
@@ -541,6 +542,7 @@ asmlinkage void __init start_kernel(void
proc_root_init();
#endif
cpuset_init();
+ delayacct_init();
check_bugs();
diff -puN /dev/null kernel/delayacct.c
--- /dev/null 2004-06-24 23:34:38.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/delayacct.c 2006-05-02 09:46:26.000000000 +0530
@@ -0,0 +1,87 @@
+/* delayacct.c - per-task delay accounting
+ *
+ * Copyright (C) Shailabh Nagar, IBM Corp. 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ */
+
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include <linux/sysctl.h>
+#include <linux/delayacct.h>
+
+int delayacct_on __read_mostly; /* Delay accounting turned on/off */
+kmem_cache_t *delayacct_cache;
+
+static int __init delayacct_setup_enable(char *str)
+{
+ delayacct_on = 1;
+ return 1;
+}
+__setup("delayacct", delayacct_setup_enable);
+
+void delayacct_init(void)
+{
+ delayacct_cache = kmem_cache_create("delayacct_cache",
+ sizeof(struct task_delay_info),
+ 0,
+ SLAB_PANIC,
+ NULL, NULL);
+ delayacct_tsk_init(&init_task);
+}
+
+void __delayacct_tsk_init(struct task_struct *tsk)
+{
+ tsk->delays = kmem_cache_zalloc(delayacct_cache, SLAB_KERNEL);
+ if (tsk->delays)
+ spin_lock_init(&tsk->delays->lock);
+}
+
+void __delayacct_tsk_exit(struct task_struct *tsk)
+{
+ kmem_cache_free(delayacct_cache, tsk->delays);
+ tsk->delays = NULL;
+}
+
+/*
+ * Start accounting for a delay statistic using
+ * its starting timestamp (@start)
+ */
+
+static inline void delayacct_start(struct timespec *start)
+{
+ do_posix_clock_monotonic_gettime(start);
+}
+
+/*
+ * Finish delay accounting for a statistic using
+ * its timestamps (@start, @end), accumalator (@total) and @count
+ */
+
+static inline void delayacct_end(struct timespec *start, struct timespec *end,
+ u64 *total, u32 *count)
+{
+ struct timespec ts = {0, 0};
+ s64 ns;
+
+ do_posix_clock_monotonic_gettime(end);
+ timespec_sub(&ts, start, end);
+ ns = timespec_to_ns(&ts);
+ if (ns < 0)
+ return;
+
+ spin_lock(¤t->delays->lock);
+ *total += ns;
+ (*count)++;
+ spin_unlock(¤t->delays->lock);
+}
+
diff -puN kernel/exit.c~delayacct-setup kernel/exit.c
--- linux-2.6.17-rc3/kernel/exit.c~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/exit.c 2006-05-02 09:44:21.000000000 +0530
@@ -35,6 +35,7 @@
#include <linux/futex.h>
#include <linux/compat.h>
#include <linux/pipe_fs_i.h>
+#include <linux/delayacct.h>
#include <asm/uaccess.h>
#include <asm/unistd.h>
@@ -910,6 +911,7 @@ fastcall NORET_TYPE void do_exit(long co
if (unlikely(tsk->compat_robust_list))
compat_exit_robust_list(tsk);
#endif
+ delayacct_tsk_exit(tsk);
exit_mm(tsk);
exit_sem(tsk);
diff -puN kernel/fork.c~delayacct-setup kernel/fork.c
--- linux-2.6.17-rc3/kernel/fork.c~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/fork.c 2006-04-28 23:47:55.000000000 +0530
@@ -44,6 +44,7 @@
#include <linux/rmap.h>
#include <linux/acct.h>
#include <linux/cn_proc.h>
+#include <linux/delayacct.h>
#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -984,6 +985,7 @@ static task_t *copy_process(unsigned lon
goto bad_fork_cleanup_put_domain;
p->did_exec = 0;
+ delayacct_tsk_init(p); /* Must remain after dup_task_struct() */
copy_flags(clone_flags, p);
p->pid = pid;
retval = -EFAULT;
diff -puN kernel/Makefile~delayacct-setup kernel/Makefile
--- linux-2.6.17-rc3/kernel/Makefile~delayacct-setup 2006-04-28 23:47:55.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/Makefile 2006-05-02 09:44:21.000000000 +0530
@@ -38,6 +38,7 @@ obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
obj-$(CONFIG_SECCOMP) += seccomp.o
obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
obj-$(CONFIG_RELAY) += relay.o
+obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o
ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
# According to Alan Modra <[email protected]>, the -fno-omit-frame-pointer is
_
Balbir Singh <[email protected]> wrote:
>
> /*
> + * sub = end - start, in normalized form
> + */
> +static inline void timespec_sub(struct timespec *start, struct timespec *end,
> + struct timespec *sub)
> +{
> + set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
> + end->tv_nsec - start->tv_nsec);
> +}
The interface might not be right here.
- I think "lhs" and "rhs" would be better names than "start" and "end".
After all, we don't _know_ that the caller is using the two times as a
start and an end. The caller might be taking the difference between two
differences, for example.
- The existing timespec and timeval funtions tend to do return-by-value.
So this would become
static inline struct timespec timespec_sub(struct timespec lhs,
struct timespec rhs)
(and given that it's inlined, the added overhead of passing the
arguments by value will be zero)
- If we don't want to do that then at least let's get the arguments in a
sane order:
static inline void timespec_sub(struct timespec *result,
struct timespec lhs, struct timespec rhs)
Balbir Singh <[email protected]> wrote:
>
> +static inline void delayacct_end(struct timespec *start, struct timespec *end,
> + u64 *total, u32 *count)
> +{
> + struct timespec ts = {0, 0};
> + s64 ns;
> +
> + do_posix_clock_monotonic_gettime(end);
> + timespec_sub(&ts, start, end);
> + ns = timespec_to_ns(&ts);
> + if (ns < 0)
> + return;
> +
> + spin_lock(¤t->delays->lock);
> + *total += ns;
> + (*count)++;
> + spin_unlock(¤t->delays->lock);
> +}
- too large to be inlined
- The initialisation of `ts' is unneeded (maybe it generated a bogus
warning, but it won't do that if you switch timespec_sub to
return-by-value)
On Mon, May 08, 2006 at 02:17:13PM -0700, Andrew Morton wrote:
> Balbir Singh <[email protected]> wrote:
> >
> > /*
> > + * sub = end - start, in normalized form
> > + */
> > +static inline void timespec_sub(struct timespec *start, struct timespec *end,
> > + struct timespec *sub)
> > +{
> > + set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
> > + end->tv_nsec - start->tv_nsec);
> > +}
>
> The interface might not be right here.
>
> - I think "lhs" and "rhs" would be better names than "start" and "end".
> After all, we don't _know_ that the caller is using the two times as a
> start and an end. The caller might be taking the difference between two
> differences, for example.
>
> - The existing timespec and timeval funtions tend to do return-by-value.
> So this would become
>
> static inline struct timespec timespec_sub(struct timespec lhs,
> struct timespec rhs)
>
> (and given that it's inlined, the added overhead of passing the
> arguments by value will be zero)
Agreed, I will make these changes.
>
> - If we don't want to do that then at least let's get the arguments in a
> sane order:
>
> static inline void timespec_sub(struct timespec *result,
> struct timespec lhs, struct timespec rhs)
>
--
Balbir Singh,
Linux Technology Center,
IBM Software Labs
On Mon, May 08, 2006 at 02:23:22PM -0700, Andrew Morton wrote:
> Balbir Singh <[email protected]> wrote:
> >
> > +static inline void delayacct_end(struct timespec *start, struct timespec *end,
> > + u64 *total, u32 *count)
> > +{
> > + struct timespec ts = {0, 0};
> > + s64 ns;
> > +
> > + do_posix_clock_monotonic_gettime(end);
> > + timespec_sub(&ts, start, end);
> > + ns = timespec_to_ns(&ts);
> > + if (ns < 0)
> > + return;
> > +
> > + spin_lock(¤t->delays->lock);
> > + *total += ns;
> > + (*count)++;
> > + spin_unlock(¤t->delays->lock);
> > +}
>
> - too large to be inlined
I will un-inline it.
>
> - The initialisation of `ts' is unneeded (maybe it generated a bogus
> warning, but it won't do that if you switch timespec_sub to
> return-by-value)
gcc-4.1 does generate a bogus warning. I will switch to return by value
and remove the initialization of `ts'
Thanks,
Balbir Singh,
Linux Technology Center,
IBM Software Labs
On Tue, 2006-05-02 at 11:42 +0530, Balbir Singh wrote:
> /*
> + * sub = end - start, in normalized form
> + */
> +static inline void timespec_sub(struct timespec *start, struct timespec *end,
> + struct timespec *sub)
> +{
> + set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
> + end->tv_nsec - start->tv_nsec);
> +}
Please use the existing ktime_t functions for that purpose. The ktime_t
format has nanosecond resolution and is optimized for 32/64bit machines.
> +static inline void delayacct_start(struct timespec *start)
> +{
> + do_posix_clock_monotonic_gettime(start);
> +}
Please get rid of this wrapper and use the ktime based functions for
that.
> +/*
> + * Finish delay accounting for a statistic using
> + * its timestamps (@start, @end), accumalator (@total) and @count
> + */
> +
> +static inline void delayacct_end(struct timespec *start, struct timespec *end,
> + u64 *total, u32 *count)
Please use ktime_t for total.
> +{
> + struct timespec ts = {0, 0};
> + s64 ns;
> +
> + do_posix_clock_monotonic_gettime(end);
> + timespec_sub(&ts, start, end);
> + ns = timespec_to_ns(&ts);
> + if (ns < 0)
> + return;
monotonic time is monotonic increasing. So delta is always >= 0 !
tglx
On Tue, May 09, 2006 at 11:46:46AM +0000, Thomas Gleixner wrote:
> On Tue, 2006-05-02 at 11:42 +0530, Balbir Singh wrote:
> > /*
> > + * sub = end - start, in normalized form
> > + */
> > +static inline void timespec_sub(struct timespec *start, struct timespec *end,
> > + struct timespec *sub)
> > +{
> > + set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
> > + end->tv_nsec - start->tv_nsec);
> > +}
>
> Please use the existing ktime_t functions for that purpose. The ktime_t
> format has nanosecond resolution and is optimized for 32/64bit machines.
>
> > +static inline void delayacct_start(struct timespec *start)
> > +{
> > + do_posix_clock_monotonic_gettime(start);
> > +}
>
> Please get rid of this wrapper and use the ktime based functions for
> that.
>
> > +/*
> > + * Finish delay accounting for a statistic using
> > + * its timestamps (@start, @end), accumalator (@total) and @count
> > + */
> > +
> > +static inline void delayacct_end(struct timespec *start, struct timespec *end,
> > + u64 *total, u32 *count)
>
> Please use ktime_t for total.
>
> > +{
> > + struct timespec ts = {0, 0};
> > + s64 ns;
> > +
> > + do_posix_clock_monotonic_gettime(end);
> > + timespec_sub(&ts, start, end);
> > + ns = timespec_to_ns(&ts);
> > + if (ns < 0)
> > + return;
>
> monotonic time is monotonic increasing. So delta is always >= 0 !
>
> tglx
>
>
>
>
I am going through the ktime interface and it seems interesting.
I will look into it and see if we can migrate the interface
to use ktime
Balbir Singh,
Linux Technology Center,
IBM Software Labs
On Mon, May 08, 2006 at 02:17:13PM -0700, Andrew Morton wrote:
> Balbir Singh <[email protected]> wrote:
> >
> > /*
> > + * sub = end - start, in normalized form
> > + */
> > +static inline void timespec_sub(struct timespec *start, struct timespec *end,
> > + struct timespec *sub)
> > +{
> > + set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
> > + end->tv_nsec - start->tv_nsec);
> > +}
>
> The interface might not be right here.
>
> - I think "lhs" and "rhs" would be better names than "start" and "end".
> After all, we don't _know_ that the caller is using the two times as a
> start and an end. The caller might be taking the difference between two
> differences, for example.
>
> - The existing timespec and timeval funtions tend to do return-by-value.
> So this would become
>
> static inline struct timespec timespec_sub(struct timespec lhs,
> struct timespec rhs)
>
> (and given that it's inlined, the added overhead of passing the
> arguments by value will be zero)
>
> - If we don't want to do that then at least let's get the arguments in a
> sane order:
>
> static inline void timespec_sub(struct timespec *result,
> struct timespec lhs, struct timespec rhs)
>
Hi, Andrew,
Please find the updated patch, which changes the interface of timespec_sub()
as suggested in the review comments
Changelog
1. Change the interface of timespec_sub() to return by value and rename
the arguments. Use lhs,rhs instead of end,start
Changes under consideration
1. Migrate to the ktime interface (Thomas Gleixner)
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Signed-off-by: Balbir Singh <[email protected]>
---
include/linux/time.h | 12 +++++++-----
kernel/delayacct.c | 2 +-
2 files changed, 8 insertions(+), 6 deletions(-)
diff -puN include/linux/time.h~timespec-sub-return-by-value include/linux/time.h
--- linux-2.6.17-rc3/include/linux/time.h~timespec-sub-return-by-value 2006-05-10 12:03:11.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/time.h 2006-05-10 12:26:44.000000000 +0530
@@ -68,13 +68,15 @@ extern unsigned long mktime(const unsign
extern void set_normalized_timespec(struct timespec *ts, time_t sec, long nsec);
/*
- * sub = end - start, in normalized form
+ * sub = lhs - rhs, in normalized form
*/
-static inline void timespec_sub(struct timespec *start, struct timespec *end,
- struct timespec *sub)
+static inline struct timespec timespec_sub(struct timespec *lhs,
+ struct timespec *rhs)
{
- set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
- end->tv_nsec - start->tv_nsec);
+ struct timespec ts_delta;
+ set_normalized_timespec(&ts_delta, lhs->tv_sec - rhs->tv_sec,
+ lhs->tv_nsec - rhs->tv_nsec);
+ return ts_delta;
}
/*
diff -puN kernel/delayacct.c~timespec-sub-return-by-value kernel/delayacct.c
--- linux-2.6.17-rc3/kernel/delayacct.c~timespec-sub-return-by-value 2006-05-10 12:03:38.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/delayacct.c 2006-05-10 14:29:28.000000000 +0530
@@ -74,7 +74,7 @@ static inline void delayacct_end(struct
s64 ns;
do_posix_clock_monotonic_gettime(end);
- timespec_sub(&ts, start, end);
+ ts = timespec_sub(end, start);
ns = timespec_to_ns(&ts);
if (ns < 0)
return;
_
On Mon, May 08, 2006 at 02:23:22PM -0700, Andrew Morton wrote:
> Balbir Singh <[email protected]> wrote:
> >
> > +static inline void delayacct_end(struct timespec *start, struct timespec *end,
> > + u64 *total, u32 *count)
> > +{
> > + struct timespec ts = {0, 0};
> > + s64 ns;
> > +
> > + do_posix_clock_monotonic_gettime(end);
> > + timespec_sub(&ts, start, end);
> > + ns = timespec_to_ns(&ts);
> > + if (ns < 0)
> > + return;
> > +
> > + spin_lock(¤t->delays->lock);
> > + *total += ns;
> > + (*count)++;
> > + spin_unlock(¤t->delays->lock);
> > +}
>
> - too large to be inlined
>
> - The initialisation of `ts' is unneeded (maybe it generated a bogus
> warning, but it won't do that if you switch timespec_sub to
> return-by-value)
Hi, Andrew,
Here is an update to un-inline delayacct_end() and remove the initialization
of ts to 0.
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Changelog
1. Remove inlining of delayacct_end(), the function is too big to be inlined
2. Remove initialization of ts.
Signed-off-by: Balbir Singh <[email protected]>
---
kernel/delayacct.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff -puN kernel/delayacct.c~remove-initialization-of-ts-and-inline kernel/delayacct.c
--- linux-2.6.17-rc3/kernel/delayacct.c~remove-initialization-of-ts-and-inline 2006-05-10 14:11:21.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/delayacct.c 2006-05-10 14:11:57.000000000 +0530
@@ -67,10 +67,10 @@ static inline void delayacct_start(struc
* its timestamps (@start, @end), accumalator (@total) and @count
*/
-static inline void delayacct_end(struct timespec *start, struct timespec *end,
+static void delayacct_end(struct timespec *start, struct timespec *end,
u64 *total, u32 *count)
{
- struct timespec ts = {0, 0};
+ struct timespec ts;
s64 ns;
do_posix_clock_monotonic_gettime(end);
_
Balbir Singh <[email protected]> wrote:
>
> Please find the updated patch, which changes the interface of timespec_sub()
> as suggested in the review comments
>
> ...
>
> /*
> - * sub = end - start, in normalized form
> + * sub = lhs - rhs, in normalized form
> */
> -static inline void timespec_sub(struct timespec *start, struct timespec *end,
> - struct timespec *sub)
> +static inline struct timespec timespec_sub(struct timespec *lhs,
> + struct timespec *rhs)
> {
I'd have thought that it would be more consistent and a saner interface to
use pass-by-value:
static inline struct timespec timespec_sub(struct timespec lhs,
struct timespec rhs)
It should generate the same code.
I mentioned this last time - did you choose to not do this for some reason,
or did it just slip past?
On Wed, May 10, 2006 at 03:24:49AM -0700, Andrew Morton wrote:
> Balbir Singh <[email protected]> wrote:
> >
> > Please find the updated patch, which changes the interface of timespec_sub()
> > as suggested in the review comments
> >
> > ...
> >
> > /*
> > - * sub = end - start, in normalized form
> > + * sub = lhs - rhs, in normalized form
> > */
> > -static inline void timespec_sub(struct timespec *start, struct timespec *end,
> > - struct timespec *sub)
> > +static inline struct timespec timespec_sub(struct timespec *lhs,
> > + struct timespec *rhs)
> > {
>
> I'd have thought that it would be more consistent and a saner interface to
> use pass-by-value:
>
> static inline struct timespec timespec_sub(struct timespec lhs,
> struct timespec rhs)
>
> It should generate the same code.
>
> I mentioned this last time - did you choose to not do this for some reason,
> or did it just slip past?
Sorry, that definitely slip past.
I'll send another update
Balbir Singh,
Linux Technology Center,
IBM Software Labs
On Wed, May 10, 2006 at 03:57:16PM +0530, Balbir Singh wrote:
> On Wed, May 10, 2006 at 03:24:49AM -0700, Andrew Morton wrote:
> > Balbir Singh <[email protected]> wrote:
> > >
> > > Please find the updated patch, which changes the interface of timespec_sub()
> > > as suggested in the review comments
> > >
> > > ...
> > >
> > > /*
> > > - * sub = end - start, in normalized form
> > > + * sub = lhs - rhs, in normalized form
> > > */
> > > -static inline void timespec_sub(struct timespec *start, struct timespec *end,
> > > - struct timespec *sub)
> > > +static inline struct timespec timespec_sub(struct timespec *lhs,
> > > + struct timespec *rhs)
> > > {
> >
> > I'd have thought that it would be more consistent and a saner interface to
> > use pass-by-value:
> >
> > static inline struct timespec timespec_sub(struct timespec lhs,
> > struct timespec rhs)
> >
> > It should generate the same code.
> >
> > I mentioned this last time - did you choose to not do this for some reason,
> > or did it just slip past?
>
> Sorry, that definitely slip past.
>
> I'll send another update
>
Hi, Andrew,
Here is another attempt to do fix the interface.
Thanks for the review,
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Changelog
1. Change the interface of timespec_sub() to return by value and rename
the arguments. Use lhs,rhs instead of end,start.
2. Pass the arguments by value
Signed-off-by: Balbir Singh <[email protected]>
---
include/linux/time.h | 12 +++++++-----
kernel/delayacct.c | 2 +-
2 files changed, 8 insertions(+), 6 deletions(-)
diff -puN include/linux/time.h~timespec-sub-return-by-value include/linux/time.h
--- linux-2.6.17-rc3/include/linux/time.h~timespec-sub-return-by-value 2006-05-10 15:58:19.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/time.h 2006-05-10 15:59:48.000000000 +0530
@@ -68,13 +68,15 @@ extern unsigned long mktime(const unsign
extern void set_normalized_timespec(struct timespec *ts, time_t sec, long nsec);
/*
- * sub = end - start, in normalized form
+ * sub = lhs - rhs, in normalized form
*/
-static inline void timespec_sub(struct timespec *start, struct timespec *end,
- struct timespec *sub)
+static inline struct timespec timespec_sub(struct timespec lhs,
+ struct timespec rhs)
{
- set_normalized_timespec(sub, end->tv_sec - start->tv_sec,
- end->tv_nsec - start->tv_nsec);
+ struct timespec ts_delta;
+ set_normalized_timespec(&ts_delta, lhs.tv_sec - rhs.tv_sec,
+ lhs.tv_nsec - rhs.tv_nsec);
+ return ts_delta;
}
/*
diff -puN kernel/delayacct.c~timespec-sub-return-by-value kernel/delayacct.c
--- linux-2.6.17-rc3/kernel/delayacct.c~timespec-sub-return-by-value 2006-05-10 15:58:19.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/delayacct.c 2006-05-10 16:00:30.000000000 +0530
@@ -74,7 +74,7 @@ static inline void delayacct_end(struct
s64 ns;
do_posix_clock_monotonic_gettime(end);
- timespec_sub(&ts, start, end);
+ ts = timespec_sub(*end, *start);
ns = timespec_to_ns(&ts);
if (ns < 0)
return;
_