2006-01-28 02:50:04

by Bart Samwel

[permalink] [raw]
Subject: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

Hi Andrew,

Here's a threesome of patches to fix up some issues with the following
sysctl values:

/proc/sys/vm/laptop_mode
/proc/sys/vm/dirty_writeback_centisecs
/proc/sys/vm/dirty_expire_centisecs

The issues:
1. The values are not range checked when they are set. They all have
a range smaller than the full integer range.
2. Conversion from these centisecond/second values is done on-the-fly
wherever they are used. This wastes some resources.
3. The conversions are done badly. Conversion from USER_HZ to HZ is done
by doing "value * USER_HZ / HZ". One day expressed in centiseconds
already causes an overflow at HZ = 250. This should use
clock_t_to_jiffies() instead.

The approach:
1. Represent everything in jiffies internally.
2. Do the conversion and range checking in the sysctl interface.

Cheers,
Bart


2006-01-28 03:56:10

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

Bart Samwel <[email protected]> wrote:
>
> Here's a threesome of patches
>

All of which were space-stuffed by your (mozilla-derived) email client and
hence are unusable by users of non-MS-wannabe email clients. They may also
be unusable by users of mozilla-based email clients, too - I don't know.

As far as I know there's no way to prevent mailnews-derived mail clients
from performing space-stuffing. I've had a bug report in against it for at
least two years and all they've done is fartarse around with it.

IOW: please switch mail clients or use text/plain attachments.

2006-01-28 04:04:40

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

On Fri, 27 Jan 2006 19:55:39 -0800 Andrew Morton wrote:

> Bart Samwel <[email protected]> wrote:
> >
> > Here's a threesome of patches
> >
>
> All of which were space-stuffed by your (mozilla-derived) email client and
> hence are unusable by users of non-MS-wannabe email clients. They may also
> be unusable by users of mozilla-based email clients, too - I don't know.
>
> As far as I know there's no way to prevent mailnews-derived mail clients
> from performing space-stuffing. I've had a bug report in against it for at
> least two years and all they've done is fartarse around with it.
>
> IOW: please switch mail clients or use text/plain attachments.

Someone kept saying that tbird would work and I kept asking how.
What I finally got was:

a. enable html-formatted email (unintuirive, I had disabled it)
b. for the body text, select Fixed Width
c. copy-and-paste the patch

I tested it one time and it worked.
But as long as Andrew accepts text/plain attachments, that
works for him. Makes it difficult to review them in some
mail clients...

---
~Randy

2006-01-28 04:09:15

by Nish Aravamudan

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

On 1/27/06, Andrew Morton <[email protected]> wrote:
> Bart Samwel <[email protected]> wrote:
> >
> > Here's a threesome of patches
> >
>
> All of which were space-stuffed by your (mozilla-derived) email client and
> hence are unusable by users of non-MS-wannabe email clients. They may also
> be unusable by users of mozilla-based email clients, too - I don't know.
>
> As far as I know there's no way to prevent mailnews-derived mail clients
> from performing space-stuffing. I've had a bug report in against it for at
> least two years and all they've done is fartarse around with it.

I think this thread claims that there is a way (although a PITA):
http://lkml.org/lkml/2005/12/27/191

I don't use TBird, personally, but Randy D. seemed satisfied with the
results in that thread.

> IOW: please switch mail clients or use text/plain attachments.

Also viable (and maybe preferred) solutions.

Thanks,
Nish

2006-01-28 04:40:01

by Paul Jackson

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

A third alternative for sending patches, a patch-bomb script,
such as one I maintain:

http://www.speakeasy.org/~pj99/sgi/sendpatchset

Read the usage in the script for its documentation.

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401

2006-01-28 11:41:04

by Bart Samwel

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds



Make that the internal values for:

/proc/sys/vm/dirty_writeback_centisecs
/proc/sys/vm/dirty_expire_centisecs

are stored as jiffies instead of centiseconds. Let the sysctl
interface do the conversions with full precision using
clock_t_to_jiffies, instead of doing overflow-sensitive on-the-fly
conversions every time the values are used.


Cons: apparent precision loss if HZ is not a multiple of 100,
because of conversion back and forth. This is a common problem
for all sysctl values that use proc_dointvec_userhz_jiffies.
(There is only one other in-tree use, in net/core/neighbour.c.)


Signed-off-by: Bart Samwel <[email protected]>

--- linux-2.6.16-rc1.orig/mm/page-writeback.c 2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/mm/page-writeback.c 2006-01-28 01:43:25.000000000 +0100
@@ -75,12 +75,12 @@
* The interval between `kupdate'-style writebacks, in centiseconds
* (hundredths of a second)
*/
-int dirty_writeback_centisecs = 5 * 100;
+int dirty_writeback_interval = 5 * HZ;

/*
* The longest number of centiseconds for which data is allowed to remain dirty
*/
-int dirty_expire_centisecs = 30 * 100;
+int dirty_expire_interval = 30 * HZ;

/*
* Flag that makes the machine dump writes/reads and block dirtyings.
@@ -379,8 +379,8 @@
* just walks the superblock inode list, writing back any inodes which are
* older than a specific point in time.
*
- * Try to run once per dirty_writeback_centisecs. But if a writeback event
- * takes longer than a dirty_writeback_centisecs interval, then leave a
+ * Try to run once per dirty_writeback_interval. But if a writeback event
+ * takes longer than a dirty_writeback_interval interval, then leave a
* one-second gap.
*
* older_than_this takes precedence over nr_to_write. So we'll only write back
@@ -405,9 +405,9 @@
sync_supers();

get_writeback_state(&wbs);
- oldest_jif = jiffies - (dirty_expire_centisecs * HZ) / 100;
+ oldest_jif = jiffies - dirty_expire_interval;
start_jif = jiffies;
- next_jif = start_jif + (dirty_writeback_centisecs * HZ) / 100;
+ next_jif = start_jif + dirty_writeback_interval;
nr_to_write = wbs.nr_dirty + wbs.nr_unstable +
(inodes_stat.nr_inodes - inodes_stat.nr_unused);
while (nr_to_write > 0) {
@@ -424,7 +424,7 @@
}
if (time_before(next_jif, jiffies + HZ))
next_jif = jiffies + HZ;
- if (dirty_writeback_centisecs)
+ if (dirty_writeback_interval)
mod_timer(&wb_timer, next_jif);
}

@@ -434,11 +434,11 @@
int dirty_writeback_centisecs_handler(ctl_table *table, int write,
struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
{
- proc_dointvec(table, write, file, buffer, length, ppos);
- if (dirty_writeback_centisecs) {
+ proc_dointvec_userhz_jiffies(table, write, file, buffer, length, ppos);
+ if (dirty_writeback_interval) {
mod_timer(&wb_timer,
- jiffies + (dirty_writeback_centisecs * HZ) / 100);
- } else {
+ jiffies + dirty_writeback_interval);
+ } else {
del_timer(&wb_timer);
}
return 0;
@@ -543,7 +543,7 @@
if (vm_dirty_ratio <= 0)
vm_dirty_ratio = 1;
}
- mod_timer(&wb_timer, jiffies + (dirty_writeback_centisecs * HZ) / 100);
+ mod_timer(&wb_timer, jiffies + dirty_writeback_interval);
set_ratelimit();
register_cpu_notifier(&ratelimit_nb);
}
--- linux-2.6.16-rc1.orig/include/linux/writeback.h 2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/include/linux/writeback.h 2006-01-28 01:34:03.000000000 +0100
@@ -88,8 +88,8 @@
/* These are exported to sysctl. */
extern int dirty_background_ratio;
extern int vm_dirty_ratio;
-extern int dirty_writeback_centisecs;
-extern int dirty_expire_centisecs;
+extern int dirty_writeback_interval;
+extern int dirty_expire_interval;
extern int block_dump;
extern int laptop_mode;

--- linux-2.6.16-rc1.orig/kernel/sysctl.c 2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/kernel/sysctl.c 2006-01-28 01:34:18.000000000 +0100
@@ -717,18 +717,18 @@
{
.ctl_name = VM_DIRTY_WB_CS,
.procname = "dirty_writeback_centisecs",
- .data = &dirty_writeback_centisecs,
- .maxlen = sizeof(dirty_writeback_centisecs),
+ .data = &dirty_writeback_interval,
+ .maxlen = sizeof(dirty_writeback_interval),
.mode = 0644,
.proc_handler = &dirty_writeback_centisecs_handler,
},
{
.ctl_name = VM_DIRTY_EXPIRE_CS,
.procname = "dirty_expire_centisecs",
- .data = &dirty_expire_centisecs,
- .maxlen = sizeof(dirty_expire_centisecs),
+ .data = &dirty_expire_interval,
+ .maxlen = sizeof(dirty_expire_interval),
.mode = 0644,
- .proc_handler = &proc_dointvec,
+ .proc_handler = &proc_dointvec_userhz_jiffies,
},
{
.ctl_name = VM_NR_PDFLUSH_THREADS,


Attachments:
laptop_mode_in_jiffies.patch (1.44 kB)
check_sysctl_jiffies.patch (1.02 kB)
dirty_centisecs_jiffies.patch (4.55 kB)
Download all attachments

2006-01-28 13:08:51

by Ingo Oeser

[permalink] [raw]
Subject: Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

Randy Dunlap wrote:
> I tested it one time and it worked.
> But as long as Andrew accepts text/plain attachments, that
> works for him. Makes it difficult to review them in some
> mail clients...

ReVIEW ist easy.

Just citing and commenting patches is difficult on mail clients not pasting
together all text/plain attachments.

That's the only technical reason so far.


Regards

Ingo Oeser