* Re: [PATCH] Document futex PI design
From: Ingo Molnar @ 2006-05-10 10:17 UTC (permalink / raw)
To: Steven Rostedt; +Cc: akpm, Thomas Gleixner, LKML
In-Reply-To: <Pine.LNX.4.58.0605100429220.436@gandalf.stny.rr.com>
* Steven Rostedt <rostedt@goodmis.org> wrote:
> +# This document is copyright 2006 by Steven Rostedt
> +# It is licensed under the GFDL version 1.2 which can be
> +# downloaded at http://www.kihontech.com/license/fdl.txt
s/at/from
in fact i'd suggest this:
> +# Copyright (c) 2006 Steven Rostedt
> +# Licensed under the GNU Free Documentation License, Version 1.2
and without the URL. (you really dont want to make a license partly
depend on an URL and thus making it potentially ambigious. Saying GFDL
1.2 is specific enough.)
Ingo
^ permalink raw reply
* Re: [PATCH] Fix console utf8 composing
From: Alexander E. Patrakov @ 2006-05-10 9:51 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Ingo Oeser, LKML
In-Reply-To: <Pine.LNX.4.61.0605100904250.27657@yvahk01.tjqt.qr>
Jan Engelhardt wrote:
>> Just for the archive...
>>
>> On Tuesday, 9. May 2006 09:49, Alexander E. Patrakov wrote:
>>> Both the current situation and my patch share the defect that an accent
>>> cannot be put on top of a multibyte character, such as Greek letter alpha.
>
> With 80x25, that [almost] would not be possible either because of the 256
> character limit. Of course the concern is totally valid for fbterm.
This concern is totally valid for the 80x25 console and the standard "gr" keymap:
compose '\'' 'α' to 'ά'
and the ά character is indeed present in the iso07.16 font and used in *.po
files from console-tools.
--
Alexander E. Patrakov
^ permalink raw reply
* [PATCH] Make SysRq work with odd keyboards
From: Fredrik Roubert @ 2006-05-10 10:18 UTC (permalink / raw)
To: linux-kernel; +Cc: Mike A. Harris
In-Reply-To: <Pine.LNX.4.21.0002260917480.966-200000@asdf.capslock.lan>
[-- Attachment #1.1: Type: text/plain, Size: 2247 bytes --]
Hi!
My keyboard is quite old, but I'm very fond of it so I wouldn't like to
replace it, even though it behaves quite odd when the SysRq key is
pressed: It sends the make and break codes immediately after another,
even when the key is beeing held down. After searching the list archives
I found that Mike A. Harris used to have a keyboard that be behaved in
the same way:
http://www.ussg.iu.edu/hypermail/linux/kernel/0001.2/1515.html
http://www.ussg.iu.edu/hypermail/linux/kernel/0001.3/0099.html
He also wrote a patch for the 2.2.14 kernel with a work-around for the
troublesome keyboard:
http://www.ussg.iu.edu/hypermail/linux/kernel/0002.3/0518.html
http://www.ussg.iu.edu/hypermail/linux/kernel/0002.3/0683.html
Using the same work-around as in Mike's patch, I've now written a new
patch (see attachment) for the 2.6.15 kernel.
The idea is quite simple: Discard the SysRq break code if Alt is still
being held down. This way the broken keyboard can send the break code
(or the user with a normal keyboard can release the SysRq key) and the
kernel waits until the next key is pressed or the Alt key is released.
Would this work-around be acceptable for inclusion in the kernel?
(I don't know how common these kinds of keyboards are, but we at least
know that they are common enough to get two people to write patches to
support them.)
In the patch, I've used the constant sysrq_fix to show where the
work-around is being invoked. If the code with the work-around is used
with a normal keyboard, SysRq will work as expected with the addition
that one can release SysRq after initially pressed. This behaviour might
not be desireable, and in Mike's original patch /proc/sys/kernel/sysrq
was used to control activation of the work-around.
The method he used to get the value from /proc/sys/kernel/sysrq doesn't
work with current kernels, so before I write a new way to configure it,
I'd like to ask you kernel developers if you think that this should be
configurable and if it should be, if it's best to use /proc or to make
it a compile-time option?
Cheers // Fredrik Roubert
--
Sörbyplan 5 | +46 8 7609169 / +46 708 776974
SE-163 71 Spånga | http://www.df.lth.se/~roubert/
[-- Attachment #1.2: linux-2.6.15-keyboard-sysrq-1.patch --]
[-- Type: text/plain, Size: 1530 bytes --]
--- linux-2.6.15/drivers/char/keyboard.c 2006-01-03 04:21:10.000000000 +0100
+++ linux-2.6.15-sysrq/drivers/char/keyboard.c 2006-03-14 00:05:48.000000000 +0100
@@ -141,6 +141,7 @@
/* Simple translation table for the SysRq keys */
#ifdef CONFIG_MAGIC_SYSRQ
+static const int sysrq_fix = 1;
unsigned char kbd_sysrq_xlate[KEY_MAX + 1] =
"\000\0331234567890-=\177\t" /* 0x00 - 0x0f */
"qwertyuiop[]\r\000as" /* 0x10 - 0x1f */
@@ -150,6 +151,7 @@
"230\177\000\000\213\214\000\000\000\000\000\000\000\000\000\000" /* 0x50 - 0x5f */
"\r\000/"; /* 0x60 - 0x6f */
static int sysrq_down;
+static int sysrq_alt_use;
#endif
static int sysrq_alt;
@@ -1044,7 +1046,7 @@
kbd = kbd_table + fg_console;
if (keycode == KEY_LEFTALT || keycode == KEY_RIGHTALT)
- sysrq_alt = down;
+ sysrq_alt = down ? keycode : 0;
#ifdef CONFIG_SPARC
if (keycode == KEY_STOP)
sparc_l1_a_state = down;
@@ -1064,9 +1066,14 @@
#ifdef CONFIG_MAGIC_SYSRQ /* Handle the SysRq Hack */
if (keycode == KEY_SYSRQ && (sysrq_down || (down == 1 && sysrq_alt))) {
- sysrq_down = down;
+ if (!sysrq_fix || !sysrq_down) {
+ sysrq_down = down;
+ sysrq_alt_use = sysrq_alt;
+ }
return;
}
+ if (sysrq_fix && sysrq_down && !down && keycode == sysrq_alt_use)
+ sysrq_down = 0;
if (sysrq_down && down && !rep) {
handle_sysrq(kbd_sysrq_xlate[keycode], regs, tty);
return;
[-- Attachment #2: Type: application/pgp-signature, Size: 303 bytes --]
^ permalink raw reply
* [PATCH][delayacct] Fix the timespec_sub() interface (was Re: [Patch 1/8] Setup)
From: Balbir Singh @ 2006-05-10 10:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan, Thomas Gleixner
In-Reply-To: <20060508141713.60c9d33e.akpm@osdl.org>
On Mon, May 08, 2006 at 02:17:13PM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> 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 <balbir@in.ibm.com>
---
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;
_
^ permalink raw reply
* Re: tcp compound
From: Angelo P. Castellani @ 2006-05-10 10:20 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20060509110118.197b9a92@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 300 bytes --]
On 5/9/06, Stephen Hemminger <shemminger@osdl.org> wrote:
> Moved discussion over to netdev mailing list..
>
> Could you export symbols in tcp_vegas (and change config dependencies) to
> allow code reuse rather than having to copy/paste everything from vegas?
I hope I've done that properly.
[-- Attachment #2: tcp_compound.patch.gz --]
[-- Type: application/x-gzip, Size: 4112 bytes --]
^ permalink raw reply
* [PATCH][delayacct] un-inline delayacct_end(), remove initialization of ts (was Re: [Patch 1/8] Setup)
From: Balbir Singh @ 2006-05-10 10:18 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan
In-Reply-To: <20060508142322.71e88a54.akpm@osdl.org>
On Mon, May 08, 2006 at 02:23:22PM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> 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 <balbir@in.ibm.com>
---
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);
_
^ permalink raw reply
* Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
From: Alan Cox @ 2006-05-10 10:34 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, linux-kernel
In-Reply-To: <200605100256.k4A2u8bd031779@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:56 -0700, Daniel Walker wrote:
> Fixes the following warnings,
>
> ipc/sem.c: In function 'sys_semctl':
> ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function
>
> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
>
> Index: linux-2.6.16/ipc/sem.c
> ===================================================================
> --- linux-2.6.16.orig/ipc/sem.c
> +++ linux-2.6.16/ipc/sem.c
> @@ -807,7 +807,7 @@ static int semctl_down(int semid, int se
> {
> struct sem_array *sma;
> int err;
> - struct sem_setbuf setbuf;
> + struct sem_setbuf setbuf = {0, 0, 0};
This causes very poor code as its initializing an object on the stack.
It also appears from inspection to be entirely un-neccessary. Instead
the compiler needs some help.
Hiding warnings like this can be a hazard as it will hide real warnings
later on.
^ permalink raw reply
* [Xenomai-core] Adeos timer interrupt handling
From: Yann.LEPROVOST @ 2006-05-10 10:23 UTC (permalink / raw)
To: xenomai
[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]
Hi all,
As I understand about adeos timer interrupt handling for the arch ARM
integrator :
- the timer is acknowledged (clearing timer interrupt bit) if there is
only one domain
- when many domains, timer interrupt bit is not cleared (I supposed to let
other domains being aware of the timer interrupt)
Concerning the AT91RM9200 target, timer interrupt works in a slightly
different way:
- the interrupt line is shared between all core peripherals (this include
system timer but also memory controller, power management...)
- to detect if the IRQ comes really from the timer, I need to read the
timer status register
The problem is that reading the timer status register automatically resets
it, clearing any interrupt bit.
Whereas the timer interrupt bit is manually controlled in the integrator
architecture, it seems that I can't do the same on the AT91RM9200
architecture.
Can anybody confirm my understanding of the adeos timer interrupt handling
?
If I'm right, then is there any other way to deal with the timer interrupt
for the AT91RM9200 ?
Last question, where can I find documentation about adeos internal
(description of functions, global variables, IRQ handling and so on) ?
Best regards
Yann Leprovost
[-- Attachment #2: Type: text/html, Size: 1839 bytes --]
^ permalink raw reply
* Re: [PATCH -mm] riva CalcStateExt gcc 4.1 warning fix
From: Alan Cox @ 2006-05-10 10:35 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, jgarzik, linux-kernel
In-Reply-To: <200605100256.k4A2u56F031749@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:56 -0700, Daniel Walker wrote:
> This could be a bug. The return from CalcVClock isn't checked
> so the variables in questions could be random data ..
>
> Fixes the following warning,
>
> drivers/video/riva/riva_hw.c: In function 'CalcStateExt':
> drivers/video/riva/riva_hw.c:1241: warning: 'p' may be used uninitialized in this function
> drivers/video/riva/riva_hw.c:1241: warning: 'n' may be used uninitialized in this function
> drivers/video/riva/riva_hw.c:1241: warning: 'm' may be used uninitialized in this function
> drivers/video/riva/riva_hw.c:1241: warning: 'VClk' may be used uninitialized in this function
But zero isn't valid data. You need to fix the missing return check not
hide the warnings. This goes for just about every patch in this series
^ permalink raw reply
* [PATCH][delayacct] Add comments on units for the delay fields (was Re: [Patch 2/8] Sync block I/O and swapin delay collection)
From: Balbir Singh @ 2006-05-10 10:20 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan
In-Reply-To: <20060508141952.2d4b9069.akpm@osdl.org>
On Mon, May 08, 2006 at 02:19:52PM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> wrote:
> >
> > @@ -550,6 +550,12 @@ struct task_delay_info {
> > * Atomicity of updates to XXX_delay, XXX_count protected by
> > * single lock above (split into XXX_lock if contention is an issue).
> > */
> > +
> > + struct timespec blkio_start, blkio_end; /* Shared by blkio, swapin */
> > + u64 blkio_delay; /* wait for sync block io completion */
> > + u64 swapin_delay; /* wait for swapin block io completion */
> > + u32 blkio_count;
> > + u32 swapin_count;
>
> These fields are a bit mystifying.
>
> In what units are blkio_delay and swapin_delay?
>
> What is the meaning behind blkio_count and swapin_count?
>
> Better comments needed, please.
Hi, Andrew,
Here is an update, that adds comments to the fields as suggested in the
review comments
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Changelog
1. Add comments to the task_delay_info structure, documenting the units
of delay and document the meaning of the count fields in the structure.
Signed-off-by: Balbir Singh <balbir@in.ibm.com>
---
include/linux/sched.h | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff -puN include/linux/sched.h~task-delay-add-comments-on-units include/linux/sched.h
--- linux-2.6.17-rc3/include/linux/sched.h~task-delay-add-comments-on-units 2006-05-10 14:35:46.000000000 +0530
+++ linux-2.6.17-rc3-balbir/include/linux/sched.h 2006-05-10 14:43:39.000000000 +0530
@@ -553,11 +553,18 @@ struct task_delay_info {
* single lock above (split into XXX_lock if contention is an issue).
*/
+ /*
+ * XXX_count is incremented on every XXX operation, the delay
+ * associated with the operation is added to XXX_delay.
+ * XXX_delay contains the accumulated delay time in nanoseconds.
+ */
struct timespec blkio_start, blkio_end; /* Shared by blkio, swapin */
u64 blkio_delay; /* wait for sync block io completion */
u64 swapin_delay; /* wait for swapin block io completion */
- u32 blkio_count;
- u32 swapin_count;
+ u32 blkio_count; /* total count of the number of sync block */
+ /* io operations performed */
+ u32 swapin_count; /* total count of the number of swapin block */
+ /* io operations performed */
};
#endif /* CONFIG_TASK_DELAY_ACCT */
_
^ permalink raw reply
* Re: [PATCH -mm] megaraid gcc 4.1 warning fix
From: Alan Cox @ 2006-05-10 10:39 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, Seokmann.Ju, linux-kernel
In-Reply-To: <200605100256.k4A2u3lB031731@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:56 -0700, Daniel Walker wrote:
> Fixes the following warning,
>
> drivers/scsi/megaraid.c: In function ‘issue_scb’:
> drivers/scsi/megaraid.c:1153: warning: passing argument 2 of ‘writel’ makes pointer from integer without a cast
And adds an exploitable memory leak. Please don't fix "bugs" blindly but
check that the error path still releases all the relevant resources.
In this case its probably sufficient just to set rval and fal through,
but each one needs to be reviewed properly.
Alan
^ permalink raw reply
* Re: [Bugme-new] [Bug 6530] New: MAINLINE
From: Paul Mackerras @ 2006-05-10 10:27 UTC (permalink / raw)
To: Andrew Morton; +Cc: bugme-daemon, netdev, xeb
In-Reply-To: <20060510023357.0d1260ee.akpm@osdl.org>
Andrew Morton writes:
> hm, a PPP fix. We seem to need some of those lately.
>
> Paul, does this look sane?
/me pages in 7 year old code...
> @@ -516,6 +516,8 @@ static void ppp_async_process(unsigned l
> /* try to push more stuff out */
> if (test_bit(XMIT_WAKEUP, &ap->xmit_flags) && ppp_async_push(ap))
> ppp_output_wakeup(&ap->chan);
> + else if (test_bit(XMIT_FULL, &ap->xmit_flags))
> + ppp_asynctty_wakeup(ap->tty);
ppp_asynctty_wakeup is supposed to be called by the serial driver when
it can take more output. It's slightly bogus having ppp_async call it
itself whether or not the serial driver can take more output at the
moment, but I suppose it won't hurt. I would really like to know the
precise circumstances where we need this fake wakeup though. Is the
serial driver failing to give us a wakeup call where it should, or is
ppp_async ignoring a wakeup for some reason?
I think the same effect could be achieved without an extra trip
through tasklet_schedule et al. by making those lines look like this
(untested):
if ((test_bit(XMIT_WAKEUP, &ap->xmit_flags) ||
test_bit(XMIT_FULL, &ap->xmit_flags)) && ppp_async_push(ap))
ppp_output_wakeup(&ap->chan);
so that ppp_async_push gets called if either XMIT_WAKEUP or XMIT_FULL
is set.
This is all relying on getting some input to kick off more output when
the wakeup gets missed, though. That's a reasonable workaround in most
situations, I guess, but I'd really like to know why the wakeup is
getting missed.
Paul.
^ permalink raw reply
* Re: [PATCH][delayacct] Fix the timespec_sub() interface (was Re: [Patch 1/8] Setup)
From: Andrew Morton @ 2006-05-10 10:24 UTC (permalink / raw)
To: balbir; +Cc: linux-kernel, lse-tech, jlan, tglx
In-Reply-To: <20060510101622.GB29432@in.ibm.com>
Balbir Singh <balbir@in.ibm.com> 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?
^ permalink raw reply
* [PATCH][delayacct] Use better names in schedstats (was Re: [Patch 3/8] cpu delay collection via schedstats)
From: Balbir Singh @ 2006-05-10 10:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan
In-Reply-To: <20060508142640.675665c7.akpm@osdl.org>
On Mon, May 08, 2006 at 02:26:40PM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> wrote:
> >
> > +/*
> > + * Expects runqueue lock to be held for atomicity of update
> > + */
> > +static inline void rq_sched_info_arrive(struct runqueue *rq,
> > + unsigned long diff)
> > +{
> > + if (rq) {
> > + rq->rq_sched_info.run_delay += diff;
> > + rq->rq_sched_info.pcnt++;
> > + }
> > +}
> > +
> > +/*
> > + * Expects runqueue lock to be held for atomicity of update
> > + */
> > +static inline void rq_sched_info_depart(struct runqueue *rq,
> > + unsigned long diff)
> > +{
> > + if (rq)
> > + rq->rq_sched_info.cpu_time += diff;
> > +}
>
> The kernel has many different units of time - jiffies, cpu ticks, ns, us,
> ms, etc. So the reader of these functions doesn't have a clue what "diff"
> is.
>
> A good way to remove all doubt in all cases is to include the units in the
> variable's name. Something like delta_jiffies, perhaps.
Hi, Andrew
I have renamed all the "diff" to "delta_jiffies" to make it easier to
read the code as suggested in the review comments.
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Changelog
1. Clean up the usage of the names. Use names with units to make the code
easier to read
Signed-off-by: Balbir Singh <balbir@in.ibm.com>
---
kernel/sched.c | 28 +++++++++++++++-------------
1 files changed, 15 insertions(+), 13 deletions(-)
diff -puN kernel/sched.c~schedstats-use-better-names kernel/sched.c
--- linux-2.6.17-rc3/kernel/sched.c~schedstats-use-better-names 2006-05-10 14:48:54.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/sched.c 2006-05-10 14:56:09.000000000 +0530
@@ -473,10 +473,10 @@ struct file_operations proc_schedstat_op
* Expects runqueue lock to be held for atomicity of update
*/
static inline void rq_sched_info_arrive(struct runqueue *rq,
- unsigned long diff)
+ unsigned long delta_jiffies)
{
if (rq) {
- rq->rq_sched_info.run_delay += diff;
+ rq->rq_sched_info.run_delay += delta_jiffies;
rq->rq_sched_info.pcnt++;
}
}
@@ -485,17 +485,19 @@ static inline void rq_sched_info_arrive(
* Expects runqueue lock to be held for atomicity of update
*/
static inline void rq_sched_info_depart(struct runqueue *rq,
- unsigned long diff)
+ unsigned long delta_jiffies)
{
if (rq)
- rq->rq_sched_info.cpu_time += diff;
+ rq->rq_sched_info.cpu_time += delta_jiffies;
}
# define schedstat_inc(rq, field) do { (rq)->field++; } while (0)
# define schedstat_add(rq, field, amt) do { (rq)->field += (amt); } while (0)
#else /* !CONFIG_SCHEDSTATS */
-static inline void rq_sched_info_arrive(struct runqueue *rq, unsigned long diff)
+static inline void rq_sched_info_arrive(struct runqueue *rq,
+ unsigned long delta_jiffies)
{}
-static inline void rq_sched_info_depart(struct runqueue *rq, unsigned long diff)
+static inline void rq_sched_info_depart(struct runqueue *rq,
+ unsigned long delta_jiffies)
{}
# define schedstat_inc(rq, field) do { } while (0)
# define schedstat_add(rq, field, amt) do { } while (0)
@@ -544,16 +546,16 @@ static inline void sched_info_dequeued(t
*/
static void sched_info_arrive(task_t *t)
{
- unsigned long now = jiffies, diff = 0;
+ unsigned long now = jiffies, delta_jiffies = 0;
if (t->sched_info.last_queued)
- diff = now - t->sched_info.last_queued;
+ delta_jiffies = now - t->sched_info.last_queued;
sched_info_dequeued(t);
- t->sched_info.run_delay += diff;
+ t->sched_info.run_delay += delta_jiffies;
t->sched_info.last_arrival = now;
t->sched_info.pcnt++;
- rq_sched_info_arrive(task_rq(t), diff);
+ rq_sched_info_arrive(task_rq(t), delta_jiffies);
}
/*
@@ -584,10 +586,10 @@ static inline void sched_info_queued(tas
*/
static inline void sched_info_depart(task_t *t)
{
- unsigned long diff = jiffies - t->sched_info.last_arrival;
+ unsigned long delta_jiffies = jiffies - t->sched_info.last_arrival;
- t->sched_info.cpu_time += diff;
- rq_sched_info_depart(task_rq(t), diff);
+ t->sched_info.cpu_time += delta_jiffies;
+ rq_sched_info_depart(task_rq(t), delta_jiffies);
}
/*
_
^ permalink raw reply
* Re: [PATCH -mm] ixj gcc 4.1 warning fix
From: Alan Cox @ 2006-05-10 10:40 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, eokerson, linux-kernel
In-Reply-To: <200605100255.k4A2tx1b031712@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:55 -0700, Daniel Walker wrote:
> Fixes the following warnings,
>
> drivers/telephony/ixj.c: In function 'ixj_pstn_state':
> drivers/telephony/ixj.c:4847: warning: 'bytes.high' may be used uninitialized in this function
> drivers/telephony/ixj.c: In function 'ixj_write_frame':
> drivers/telephony/ixj.c:3448: warning: 'blankword.high' may be used uninitialized in this function
> drivers/telephony/ixj.c:3448: warning: 'blankword.low' may be used uninitialized in this function
>
> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
No this is not a good idea either. The missing default case stuff is as
far as I can see from inspection not neccessary. If anything you want to
add
default:
BUG();
since those are impossible cases.
The unusued variables also appear to be just compiler confusion.
^ permalink raw reply
* Re: [PATCH] [MIPS] create consistency in "system type" selection
From: Ralf Baechle @ 2006-05-10 10:28 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Martin Michlmayr, Linux/MIPS Development
In-Reply-To: <Pine.LNX.4.62.0605101026450.17487@pademelon.sonytel.be>
On Wed, May 10, 2006 at 10:27:25AM +0200, Geert Uytterhoeven wrote:
> On Tue, 9 May 2006, Martin Michlmayr wrote:
> > The "system type" Kconfig options on MIPS are not consistent. For
> > some platforms, only the name is listed while other entries are
> > prepended with "Support for". Remove this as it doesn't make sense
> > when describing the "system type". This is in line with how e.g.
> > ARM handles this.
>
> I guess the `Support for' prefix came from the era you could compile one kernel
> that supported multiple systems.
You still can but they need to be lumped together into a single group
in the "System type" menu. In reality it's not terribly interesting
and rarely tested.
Ralf
^ permalink raw reply
* [PATCH][delayacct] Clean up coding style in taskstats interface (was Re: [updated] [Patch 5/8] taskstats interface)
From: Balbir Singh @ 2006-05-10 10:26 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan
In-Reply-To: <20060508143139.2f1c7623.akpm@osdl.org>
On Mon, May 08, 2006 at 02:31:39PM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> wrote:
> >
> > +
> > + if ((rc = genl_register_ops(&family, &taskstats_ops)) < 0)
> > + goto err;
>
> rc = genl_register_ops(&family, &taskstats_ops);
> if (rc < 0)
> goto err;
>
> please.
Sure, here you go Andrew
I have updated the coding style as per your suggestion
Balbir Singh,
Linux Technology Center,
IBM Software Labs
Changelog
1. Split the complex if condition into
a. function call
b. check return status for error
Signed-off-by: Balbir Singh <balbir@in.ibm.com>
---
kernel/taskstats.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletion(-)
diff -puN kernel/taskstats.c~taskstats-interface-coding-style-cleanup kernel/taskstats.c
--- linux-2.6.17-rc3/kernel/taskstats.c~taskstats-interface-coding-style-cleanup 2006-05-10 15:08:53.000000000 +0530
+++ linux-2.6.17-rc3-balbir/kernel/taskstats.c 2006-05-10 15:09:50.000000000 +0530
@@ -322,7 +322,8 @@ static int __init taskstats_init(void)
return rc;
family_registered = 1;
- if ((rc = genl_register_ops(&family, &taskstats_ops)) < 0)
+ rc = genl_register_ops(&family, &taskstats_ops);
+ if (rc < 0)
goto err;
return 0;
_
^ permalink raw reply
* Re: [PATCH -mm] idetape gcc 4.1 warning fix
From: Alan Cox @ 2006-05-10 10:42 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, linux-kernel
In-Reply-To: <200605100255.k4A2twOE031688@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:55 -0700, Daniel Walker wrote:
> Fixes the following warning,
>
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_from_user’:
> drivers/ide/ide-tape.c:2662: warning: ignoring return value of ‘copy_from_user’, declared with attribute warn_unused_result
> drivers/ide/ide-tape.c: In function ‘idetape_copy_stage_to_user’:
> count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> + WARN_ON(copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count));
So you want to let users spew all over the kernel log when a copy from
user fails. Either fix it properly or leave it alone. In this case its
actually quite hard to fix properly which is why it hasn't been done.
(POSIX doesn't require invalid addresses reliably report -EFAULT)
^ permalink raw reply
* Re: [PATCH][delayacct] Fix the timespec_sub() interface (was Re: [Patch 1/8] Setup)
From: Balbir Singh @ 2006-05-10 10:27 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, lse-tech, jlan, tglx
In-Reply-To: <20060510032449.2872a8ba.akpm@osdl.org>
On Wed, May 10, 2006 at 03:24:49AM -0700, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> 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
^ permalink raw reply
* Re: [RFC] [PATCH] Execute PCI quirks on resume from suspend-to-RAM
From: Carl-Daniel Hailfinger @ 2006-05-10 10:30 UTC (permalink / raw)
To: Pavel Machek; +Cc: Linux Kernel Mailing List, trenn, thoenig, Greg KH
In-Reply-To: <20060510093942.GA12259@elf.ucw.cz>
Pavel Machek wrote:
> Hi!
>
>> machines with the asus_hides_smbus "feature" have the problem
>> that the smbus is disabled again after suspend-to-RAM. This
>> causes all sorts of problems, the worst being a total fan
>> failure on my Samsung P35 notebook after STR and STD.
>
> What happens if we disable hiding altogether? ASUS decided software
> should not see smbus, perhaps they had a reason?
IIRC this was introduced only to keep older ms-windows versions
from complaining about hardware for which no driver existed.
Newer ms-windows versions seem to unhide the smbus like we do.
> If we decide that we want to keep unhiding, redoing quirks after
> resume is probably neccessary...
Yes. Now the question is where exactly we want to execute these
quirks. Before or after restoring pci config space? If we do it
before restoring config space, it might cause some yet unknown
side effects. If we do it after restoring config space, it might
be worse because we would restore config space of a device not
existing anymore.
Thinking about it again, if we restored the full pci config space
on resume, this quirk handling would be completely unnecessary.
Any reasons why we don't do that?
>> References: Novell bugzilla #173420.
>>
>> This (totally ugly) patch fixes it.
>> Comments/criticism welcome.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
^ permalink raw reply
* Re: [PATCH -mm] BusLogic gcc 4.1 warning fixes
From: Alan Cox @ 2006-05-10 10:44 UTC (permalink / raw)
To: Daniel Walker; +Cc: akpm, linux-kernel
In-Reply-To: <200605100255.k4A2tuuf031655@dwalker1.mvista.com>
On Maw, 2006-05-09 at 19:55 -0700, Daniel Walker wrote:
> I just commented out BusLogic_AbortCommand because the code that uses it is
> commented out the same way .. It could just be removed .
Adds another leak in the failure case.
Agree about the AbortCommand function.
^ permalink raw reply
* Re: [PATCH] smbfs: Fix slab corruption in samba error path
From: Jan Niehusmann @ 2006-05-10 10:32 UTC (permalink / raw)
To: Andrew Morton; +Cc: urban, linux-kernel, samba
In-Reply-To: <20060510022529.24e15d28.akpm@osdl.org>
On Wed, May 10, 2006 at 02:25:29AM -0700, Andrew Morton wrote:
> I think the bug is actually that this code is accessing *req after having
> doen the smb_rput(). I worry that your patch "fixes" this by accidentally
> leaking the request.
>
> We can fairly simply restructure this code so that it doesn't touch the
> request after that possible smb_rput().
>
> How does this look? If "OK", are you able to test it?
No, it doesn't look ok: The callers of smb_add_request (which are all in
fs/smbfs/proc.c) do touch the req structure after calling
smb_add_request, even if an error is returned. So your code would still
cause access after release and double frees on the req object.
As I understand the code smb_add_request is not allowed to completely
release the req structure at all.
What smb_add_request should do is
- increase the req usage counter by one (by calling smb_rget), and add
the req to one of the work queues
- or leave the usage counter alone, and don't add the req to one of the
work queues
On error, one has to be careful: If we actively remove the req from the
work queues again, we have to decrease the usage counter (otherwise we
leak requests). But if some other function already removed the req from
the queue, that function already did decrease the counter, so we are not
allowed to do it again.
The original code did get the latter case partly wrong. It assumed that
the only way a req could be removed from the work queue would be in
smb_request_recv, where the SMB_REQ_RECEIVED flag gets set. But it did
miss the error cases in smbiod.c, eg. smbiod_flush(), where the req gets
removed from the queues (and the usage counter decreased), without the
SMB_REQ_RECEIVED flag being set.
Therefore I changed the code to not check SMB_REQ_RECEIVED, but test if
the req is still on one of the work queue linked lists.
After that change, smb_add_request never releases the req, so reordering
is not necessary.
Unfortunately it's not easy to test the patch: Of course I did check
that it properly compiles, but the bug is not easily reproducible.
Jan
^ permalink raw reply
* Re: [RFC PATCH 15/35] subarch support for controlling interrupt delivery
From: Christian Limpach @ 2006-05-10 10:35 UTC (permalink / raw)
To: Andi Kleen
Cc: xen-devel, Ian Pratt, linux-kernel, Martin J. Bligh, Chris Wright,
virtualization
In-Reply-To: <200605092356.28818.ak@suse.de>
On Tue, May 09, 2006 at 11:56:28PM +0200, Andi Kleen wrote:
>
> > Everything[1] in line:
> > -rwxr-xr-x 1 cl349 cl349 2633640 May 9 19:42 vmlinux-inline-stripped
> > Everything out of line:
> > -rwxr-xr-x 1 cl349 cl349 2621352 May 9 19:45 vmlinux-outline-stripped
> >
> > Additionally, I changed did a build with only __sti and __restore_flags
> > out of line and the others in line:
> > -rwxr-xr-x 1 cl349 cl349 2617256 May 9 19:50 vmlinux-hybrid-stripped
> >
> > __sti and __restore_flags are the ones which generate more code,
> > so it seemed more sensible to make the out of line.
> >
> > Any conlusions?
>
> It looks like hybrid is a clear winner at least from the code size, isn't it?
Yes, which is why I measured that one as well.
Now, the original concern was that we have the five operations implemented
as multi-line macros and doing a hybrid solution doesn't really address
that.
Also, it's not quite clear to me what's the best way to turn three of
the five into functions, whether inline or not.
For measuring the sizes, I did the following:
add void ___restore_flags(unsigned long *x) with the implementation
and then:
#define __restore_flags(x) ___restore_flags(&(x))
Alternatively, would it make sense to change __restore_flags to take
a pointer to flags instead? That would be quite an invasive change...
Any thoughts?
christian
^ permalink raw reply
* Re: [RFC PATCH 15/35] subarch support for controlling interrupt delivery
From: Christian Limpach @ 2006-05-10 10:35 UTC (permalink / raw)
To: Andi Kleen
Cc: virtualization, Martin J. Bligh, Chris Wright, xen-devel,
linux-kernel, Ian Pratt
In-Reply-To: <200605092356.28818.ak@suse.de>
On Tue, May 09, 2006 at 11:56:28PM +0200, Andi Kleen wrote:
>
> > Everything[1] in line:
> > -rwxr-xr-x 1 cl349 cl349 2633640 May 9 19:42 vmlinux-inline-stripped
> > Everything out of line:
> > -rwxr-xr-x 1 cl349 cl349 2621352 May 9 19:45 vmlinux-outline-stripped
> >
> > Additionally, I changed did a build with only __sti and __restore_flags
> > out of line and the others in line:
> > -rwxr-xr-x 1 cl349 cl349 2617256 May 9 19:50 vmlinux-hybrid-stripped
> >
> > __sti and __restore_flags are the ones which generate more code,
> > so it seemed more sensible to make the out of line.
> >
> > Any conlusions?
>
> It looks like hybrid is a clear winner at least from the code size, isn't it?
Yes, which is why I measured that one as well.
Now, the original concern was that we have the five operations implemented
as multi-line macros and doing a hybrid solution doesn't really address
that.
Also, it's not quite clear to me what's the best way to turn three of
the five into functions, whether inline or not.
For measuring the sizes, I did the following:
add void ___restore_flags(unsigned long *x) with the implementation
and then:
#define __restore_flags(x) ___restore_flags(&(x))
Alternatively, would it make sense to change __restore_flags to take
a pointer to flags instead? That would be quite an invasive change...
Any thoughts?
christian
^ permalink raw reply
* Re: [Xenomai-core] [bug] zombie mutex owners
From: Philippe Gerum @ 2006-05-10 10:40 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai
In-Reply-To: <4461BB5A.3010403@domain.hid>
Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>
>>Hi Jan,
>>
>>
>>>running the attached test case for the native skin, you will get an ugly
>>>lock-up on probably all Xenomai versions. Granted, this code is a bit
>>>synthetic. I originally thought I could trigger the bug also via
>>>timeouts when waiting on mutexes, but this scenario is safe (the timeout
>>>is cleared before being able to cause harm).
>>>
>>
>>just in order to educate me as probably I might have got something
>>wrong at the first glance :)
>>
>>if we take this one:
>>
>>--- mutex.c 2006-02-27 15:34:58.000000000 +0100
>>+++ mutex-NEW.c 2006-05-10 11:55:25.000000000 +0200
>>@@ -391,7 +391,7 @@ int rt_mutex_lock (RT_MUTEX *mutex,
>> err = -EIDRM; /* Mutex deleted while pending. */
>> else if (xnthread_test_flags(&task->thread_base,XNTIMEO))
>> err = -ETIMEDOUT; /* Timeout.*/
>>- else if (xnthread_test_flags(&task->thread_base,XNBREAK))
>>+ else if (xnthread_test_flags(&task->thread_base,XNBREAK) &&
>>mutex->owner != task)
>> err = -EINTR; /* Unblocked.*/
>>
>> unlock_and_exit:
>>
>>As I understand task2 has a lower prio and that's why
>>
>>[task1] rt_mutex_unlock
>>[task 1] rt_task_unblock(task1)
>>
>>are called in a row.
>>
>>ok, task2 wakes up in rt_mutex_unlock() (when task1 is blocked on
>>rt_mutex_lock()) and finds XNBREAK flag but,
>>
>>[doc] -EINTR is returned if rt_task_unblock() has been called for the
>>waiting task (1) before the mutex has become available (2).
>>
>>(1) it's true, task2 was still waiting at that time;
>>(2) it's wrong, task2 was already the owner.
>>
>>So why just not to bail out XNBREAK and continue task2 as it has a
>>mutex (as shown above) ?
>
>
> Indeed, this solves the issue more gracefully.
>
> Looking at this again from a different perspective and running the test
> case with your patch in a slightly different way, I think I
> misinterpreted the crash. If I modify task2 like this
>
> void task2_fnc(void *arg)
> {
> printf("started task2\n");
> if (rt_mutex_lock(&mtx, 0) < 0) {
> printf("lock failed in task2\n");
> return;
> }
> // rt_mutex_unlock(&mtx);
>
> printf("done task2\n");
> }
>
> I'm also getting a crash. So the problem seems to be releasing a mutex
> ownership on task termination. Well, this needs further examination.
>
> Looks like the issue is limited to cleanup problems and is not that
> widespread to other skins as I thought. RTDM is not involved as it does
> not know EINTR for rtdm_mutex_lock. The POSIX skins runs in a loop on
> interruption and should recover from this.
>
> Besides this, we then may want to consider if introducing a pending
> ownership of synch objects is worthwhile to improve efficiency of PIP
> users. Not critical, but if it comes at a reasonable price... Will try
> to draft something.
>
I've planned to work over the simulator asap to implement the stealing
of ownership at the nucleus level, so that this kind of issue will
become history.
--
Philippe.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.