http://groups.google.com/group/linux.kernel?hl=en
Today's topics:
* Improving OOM killer - 2 messages, 2 authors
http://groups.google.com/group/linux.kernel/t/389db2dcf6479d30?hl=en
* Need help tracking down memory consumption increase - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/a65cc1e4910a7a13?hl=en
* mtrr/state.c unused? - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/55f2d38c4dc1870d?hl=en
* netfilter: per netns nf_conntrack_cachep - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/8498db207556ff69?hl=en
* fec: fix uninitialized rx buffer usage - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/281ca1a532ca6d76?hl=en
* scripts/get_maintainer.pl: add ability to read from STDIN - 1 messages, 1
author
http://groups.google.com/group/linux.kernel/t/a4a26eb422c542dc?hl=en
* regression in 2.6.27.45 with usb and suspend-to-disk - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/f18f89f344fa881c?hl=en
* ice1712: fix: lock samplerate when samplerate locking is enabled. - 1
messages, 1 author
http://groups.google.com/group/linux.kernel/t/732fd469186a0b81?hl=en
* enhanced reimplemention of the kfifo API - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/21e9c147c9adf137?hl=en
* syslog: use defined constants instead of raw numbers - 2 messages, 1 author
http://groups.google.com/group/linux.kernel/t/e1594790606b7db6?hl=en
* perf/trace/lock optimization/scalability improvements - 3 messages, 2
authors
http://groups.google.com/group/linux.kernel/t/88914be5e6b28d7d?hl=en
* MX1/MX2: -EINVAL overwritten in second iteration in mxc_gpio_setup_multiple_
pins() - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/7de8eb855916e1be?hl=en
* Problem with set_memory_rw() - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/f2e3a6a90d18e63c?hl=en
* pps: add parallel port PPS signal generator - 2 messages, 1 author
http://groups.google.com/group/linux.kernel/t/5b7b716ca20e757a?hl=en
* pps: time synchronization over LPT - 4 messages, 1 author
http://groups.google.com/group/linux.kernel/t/0c8202dba360df9c?hl=en
* emulate accessed bit for EPT - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/f7a13e09a6e2aadc?hl=en
* [2/4] SLAB: Set up the l3 lists for the memory of freshly added memory - 2
messages, 1 author
http://groups.google.com/group/linux.kernel/t/4398545cf5dbd1de?hl=en
==============================================================================
TOPIC: Improving OOM killer
http://groups.google.com/group/linux.kernel/t/389db2dcf6479d30?hl=en
==============================================================================
== 1 of 2 ==
Date: Wed, Feb 3 2010 12:30 pm
From: David Rientjes
On Wed, 3 Feb 2010, Frans Pop wrote:
> That doesn't take into account:
> - applications where the oom_adj value is hardcoded to a specific value
> (for whatever reason)
> - sysadmin scripts that set oom_adj from the console
>
The fundamentals are the same: negative values mean the task is less
likely to be preferred and positive values mean the task is more likely,
only the scale is different. That scale is exported by the kernel via
OOM_ADJUST_MIN and OOM_ADJUST_MAX and has been since 2006. I don't think
we need to preserve legacy applications or scripts that use hardcoded
values without importing linux/oom.h.
> I would think that oom_adj is a documented part of the userspace ABI and
> that the change you propose does not fit the normal backwards
> compatibility requirements for exposed tunables.
>
The range is documented (but it should have been documented as being from
OOM_ADJUST_MIN to OOM_ADJUST_MAX) but its implementation as a bitshift is
not; it simply says that positive values mean the task is more preferred
and negative values mean it is less preferred. Those semantics are
preserved.
> I think that at least any user who's currently setting oom_adj to -17 has a
> right to expect that to continue to mean "oom killer disabled". And for
> any other value they should get a similar impact to the current impact,
> and not one that's reduced by a factor 66.
>
If the baseline changes as we all agree it needs to such that oom_adj no
longer represents the same thing it did in the first place (it would
become a linear bias), I think this breakage is actually beneficial.
Users will now be able to tune their oom_adj values based on a fraction of
system memory to bias their applications either preferrably or otherwise.
I think we should look at Linux over the next couple of years and decide
if we want to be married to the current semantics of oom_adj that are
going to change (as it would require being a factor of 66, as you
mentioned) when the implementation it was designed for has vanished.
--
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/
== 2 of 2 ==
Date: Wed, Feb 3 2010 1:30 pm
From: Lubos Lunak
On Wednesday 03 of February 2010, Balbir Singh wrote:
> * Lubos Lunak <l.lunak@suse.cz> [2010-02-03 13:10:27]:
> > On Wednesday 03 of February 2010, Balbir Singh wrote:
> > > 2. RSS alone is not sufficient, RSS does not account for shared pages,
> > > so we ideally need something like PSS.
> >
> > Just to make sure I understand what you mean with "RSS does not account
> > for shared pages" - you say that if a page is shared by 4 processes, then
> > when calculating badness for them, only 1/4 of the page should be counted
> > for each? Yes, I suppose so, that makes sense.
>
> Yes, that is what I am speaking of
>
> > That's more like fine-tunning at
> > this point though, as long as there's no agreement that moving away from
> > VmSize is an improvement.
>
> There is no easy way to calculate the Pss today without walking the
> page tables, but some simplification there will make it a better and a
> more accurate metric.
OOM should be a rare situation, so doing a little amount of counting
shouldn't be a big deal. Especially if the machine is otherwise busy waiting
for the HDD paging stuff out and in again and has plenty of CPU time to
waste.
--
Lubos Lunak
openSUSE Boosters team, KDE developer
l.lunak@suse.cz , l.lunak@kde.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/
==============================================================================
TOPIC: Need help tracking down memory consumption increase
http://groups.google.com/group/linux.kernel/t/a65cc1e4910a7a13?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:30 pm
From: "Chris Friesen"
I'm seeing a slow increase in memory consumption and I'm trying to
narrow down the possible causes.
In my test, I got the system into steady-state and then started
monitoring /proc/meminfo.
After 14 hrs:
MemFree had dropped by 594MB.
Active + Inactive + Slab increased by 594MB
Buffers + Cached + AnonPages + Mapped + Slab increased by 290MB
The other categories in /proc/meminfo didn't change significantly.
I've done some experimenting and it seems that pages allocated in the
kernel via alloc_page() and friends don't show up in /proc/meminfo
except that they're deducted from the MemFree category. Specifically,
they don't seem to show up in the Active/Inactive category. Can someone
confirm this?
Given the above, what types of pages are in Active/Inactive other than
Buffers + Cached + AnonPages + Mapped?
Thanks,
Chris
--
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/
==============================================================================
TOPIC: mtrr/state.c unused?
http://groups.google.com/group/linux.kernel/t/55f2d38c4dc1870d?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:30 pm
From: Borislav Petkov
It looks like <arch/x86/kernel/cpu/mtrr/state.c> is not used by
anything in the kernel.
Besides cleanups, the only relevant patch in the git history is:
commit 2ec1df4130c60d1eb49dc0fa0ed15858fede6b05
Author: Thomas Gleixner <tglx@linutronix.de>
Date: Thu Oct 11 11:16:28 2007 +0200
i386: move kernel/cpu/mtrr
which adds it. Hmm...
A
git log -i --pickaxe-all -Sset_mtrr_prepare_save
for example, shows that
commit 9a6b344ea967efa0bb5ca4cb5405f840652b66c4
Author: Harvey Harrison <harvey.harrison@gmail.com>
Date: Mon Feb 4 16:48:01 2008 +0100
x86: remove long dead cyrix mtrr code
has removed the last callers of those helpers and they're now unused.
They should go if we don't need them anymore.
--
Regards/Gruss,
Boris.
--
Advanced Micro Devices, Inc.
Operating Systems Research Center
--
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/
==============================================================================
TOPIC: netfilter: per netns nf_conntrack_cachep
http://groups.google.com/group/linux.kernel/t/8498db207556ff69?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:30 pm
From: Jon Masters
On Wed, 2010-02-03 at 13:10 +0100, Patrick McHardy wrote:
> Jon, could you give this patch a try please?
> plain text document attachment (x)
> commit 056ff3e3bd1563969a311697323ff929df94415c
> Author: Patrick McHardy <kaber@trash.net>
> Date: Wed Feb 3 12:58:06 2010 +0100
Patrick, can I regard this as the official fix for 2.6.33?
Jon.
--
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/
==============================================================================
TOPIC: fec: fix uninitialized rx buffer usage
http://groups.google.com/group/linux.kernel/t/281ca1a532ca6d76?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:30 pm
From: Grant Likely
On Wed, Feb 3, 2010 at 11:38 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 3, 2010 at 11:33 AM, Amit Kucheria
> <amit.kucheria@canonical.com> wrote:
>> On Wed, Feb 3, 2010 at 8:46 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>
>>> fec related patches 8 & 9 look okay to me.
>>>
>>> g.
>>
>> Can I take that as an Acked-by?
>
> of course.
BTW, since these 2 patches are essentially independent, you may find
it easier to get all the patches merged if you post the FEC changes in
a separate patch series.
g.
--
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/
==============================================================================
TOPIC: scripts/get_maintainer.pl: add ability to read from STDIN
http://groups.google.com/group/linux.kernel/t/a4a26eb422c542dc?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:40 pm
From: Joe Perches
On Wed, 2010-02-03 at 08:27 +0100, Borislav Petkov wrote:
> On Tue, Feb 02, 2010 at 09:35:47PM -0800, Joe Perches wrote:
> > I think it's better if the dash isn't required.
> >
> > http://patchwork.kernel.org/patch/69650/
>
> Yeah, about that, when are you going to send it upstream? Or do you want
> me to do that?
I already sent it to Andrew Morton but I
think he was away for the holidays then.
Andrew, can you pick this one up please for
the next not-rc release?
Doesn't need or accept '-' as a trailing option to read stdin.
Doesn't print usage() after bad options.
Adds --usage as command line equivalent of --help
Suggested-by: Borislav Petkov <petkovbb@googlemail.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 445e884..9fe4628 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -122,7 +122,7 @@ if (!GetOptions(
'k|keywords!' => \$keywords,
'f|file' => \$from_filename,
'v|version' => \$version,
- 'h|help' => \$help,
+ 'h|help|usage' => \$help,
)) {
die "$P: invalid argument - use --help if necessary\n";
}
@@ -137,9 +137,9 @@ if ($version != 0) {
exit 0;
}
-if ($#ARGV < 0) {
- usage();
- die "$P: argument missing: patchfile or -f file please\n";
+if (-t STDIN && !@ARGV) {
+ # We're talking to a terminal, but have no command line arguments.
+ die "$P: missing patchfile or -f file - use --help if necessary\n";
}
if ($output_separator ne ", ") {
@@ -152,14 +152,12 @@ if ($output_rolestats) {
my $selections = $email + $scm + $status + $subsystem + $web;
if ($selections == 0) {
- usage();
die "$P: Missing required option: email, scm, status, subsystem or web\n";
}
if ($email &&
($email_maintainer + $email_list + $email_subscriber_list +
$email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
- usage();
die "$P: Please select at least 1 email option\n";
}
@@ -233,12 +231,18 @@ my @files = ();
my @range = ();
my @keyword_tvi = ();
+if (!@ARGV) {
+ push(@ARGV, "&STDIN");
+}
+
foreach my $file (@ARGV) {
- ##if $file is a directory and it lacks a trailing slash, add one
- if ((-d $file)) {
- $file =~ s@([^/])$@$1/@;
- } elsif (!(-f $file)) {
- die "$P: file '${file}' not found\n";
+ if ($file ne "&STDIN") {
+ ##if $file is a directory and it lacks a trailing slash, add one
+ if ((-d $file)) {
+ $file =~ s@([^/])$@$1/@;
+ } elsif (!(-f $file)) {
+ die "$P: file '${file}' not found\n";
+ }
}
if ($from_filename) {
push(@files, $file);
--
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/
==============================================================================
TOPIC: regression in 2.6.27.45 with usb and suspend-to-disk
http://groups.google.com/group/linux.kernel/t/f18f89f344fa881c?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:40 pm
From: Alan Stern
On Wed, 3 Feb 2010, Corey Wright wrote:
> the device is a usb SD reader w/ SD card in it.
>
> # cat /sys/bus/usb/devices/1-8/product
> USB 2.0 SD MMC READER
>
> i removed the media reader after the failed suspend (it is only used at
> boot-up to hold the LUKS key material to decrypt the filesystem) but the
> message is the same:
>
> [ 4002.585329] ehci_hcd 0000:00:10.4: suspend root hub
> [ 4002.585334] ehci_hcd 0000:00:10.4: suspend failed because port 8 is
> resuming
> [ 4002.585338] usb usb1: bus suspend fail, err -16
>
> and /sys/bus/usb/devices/1-8 no longer exists (little
> alone /sys/bus/usb/devices/1-8/power/wakeup):
>
> # ls -1 /sys/bus/usb/devices/
> 1-0:1.0
> 1-7
> 1-7:1.0
> 2-0:1.0
> 2-1
> 2-1:1.0
> 3-0:1.0
> 4-0:1.0
> 5-0:1.0
> usb1
> usb2
> usb3
> usb4
> usb5
>
> > And what happens if you do:
> >
> > echo disabled >/sys/bus/usb/devices/1-8/power/wakeup
> >
> > before trying to hibernate?
>
> it's the same immediately after a reboot (and before suspending), a
> successful suspend, or a failed suspend:
>
> # echo disabled >/sys/bus/usb/devices/1-8/power/wakeup
> bash: echo: write error: Invalid argument
>
> and as previously said, that sysfs entry does not exist if the media reader
> is removed, though the error appears to continue to refer to that
> port/device.
Okay, thanks for testing. That narrows it down, and I believe the
patch below will fix the bug. Let me know how it works.
Alan Stern
Index: 2.6.27.45/drivers/usb/host/ehci-hub.c
===================================================================
--- 2.6.27.45.orig/drivers/usb/host/ehci-hub.c
+++ 2.6.27.45/drivers/usb/host/ehci-hub.c
@@ -255,7 +255,6 @@ static int ehci_bus_resume (struct usb_h
temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
if (test_bit(i, &ehci->bus_suspended) &&
(temp & PORT_SUSPEND)) {
- ehci->reset_done [i] = jiffies + msecs_to_jiffies (20);
temp |= PORT_RESUME;
}
ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
--
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/
==============================================================================
TOPIC: ice1712: fix: lock samplerate when samplerate locking is enabled.
http://groups.google.com/group/linux.kernel/t/732fd469186a0b81?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:50 pm
From: Sebastien Alaiwan
Hello,
I found that the sampling rate locking setting of the ice1712 sound driver was
only half-respected : when the driver was locked to, let's say, 44100Hz, and a
usermode app was requesting 48000Hz playback, the request was succesful although
the soundcard would continue to run at 44100Hz.
Here's a patch that will make those requests to fail.
diff --git a/sound/pci/ice1712/ice1712.c b/sound/pci/ice1712/ice1712.c
index c7cff6f..4775626 100644
--- a/sound/pci/ice1712/ice1712.c
+++ b/sound/pci/ice1712/ice1712.c
@@ -1181,6 +1181,8 @@ static int snd_ice1712_playback_pro_open(struct snd_pcm_substream *substream)
snd_pcm_hw_constraint_msbits(runtime, 0, 32, 24);
snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &hw_constraints_rates);
+ if (is_pro_rate_locked(ice))
+ snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_RATE, PRO_RATE_DEFAULT, PRO_RATE_DEFAULT);
if (ice->spdif.ops.open)
ice->spdif.ops.open(ice, substream);
@@ -1197,6 +1199,9 @@ static int snd_ice1712_capture_pro_open(struct snd_pcm_substream *substream)
snd_pcm_set_sync(substream);
snd_pcm_hw_constraint_msbits(runtime, 0, 32, 24);
snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &hw_constraints_rates);
+ if (is_pro_rate_locked(ice))
+ snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_RATE, PRO_RATE_DEFAULT, PRO_RATE_DEFAULT);
+
return 0;
}
--
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/
==============================================================================
TOPIC: enhanced reimplemention of the kfifo API
http://groups.google.com/group/linux.kernel/t/21e9c147c9adf137?hl=en
==============================================================================
== 1 of 1 ==
Date: Wed, Feb 3 2010 12:50 pm
From: "Ira W. Snyder"
On Wed, Feb 03, 2010 at 12:05:04PM -0800, Andrew Morton wrote:
> On Wed, 27 Jan 2010 14:00:43 +0100
> Stefani Seibold <stefani@seibold.net> wrote:
>
> > This is a complete reimplementation of the new kfifo API, which is now
> > really generic, type save and type definable.
> >
> >
> > ...
> >
> > diff -u -N -r -p linux-2.6.33-rc5.dummy/include/linux/kfifo.h linux-2.6.33-rc5.new/include/linux/kfifo.h
> > --- linux-2.6.33-rc5.dummy/include/linux/kfifo.h 1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.33-rc5.new/include/linux/kfifo.h 2010-01-27 13:34:06.324991898 +0100
> >
> > ...
> >
> > +/*
> > + * DECLARE_KFIFO_PTR - macro to declare a fifo pointer object
> > + * @fifo: name of the declared fifo
> > + * @type: type of the fifo elements
> > + * @size: the number of elements in the fifo, this must be a power of 2.
> > + */
>
> Throughout this patch the comments are in kerneldoc form, but they
> don't use the /** opening token, so the kerneldoc tools will ignore them.
>
> Please fix that up and test it.
>
> > +#define DECLARE_KFIFO_PTR(fifo, type) STRUCT_KFIFO_PTR(type) fifo
> > +
> > +/* helper macro */
> > +#define __is_kfifo_ptr(fifo) (sizeof(*fifo) == sizeof(struct __kfifo))
>
> This is weird. It's a compile-time constant. What's it here for?
>
> > +
> > +#define __kfifo_initializer(fifo) \
> > + (typeof(fifo)) { \
> > + { \
> > + { \
> > + .in = 0, \
> > + .out = 0, \
> > + .mask = __is_kfifo_ptr(&fifo) ? \
> > + 0 : \
> > + sizeof((fifo).buf)/sizeof(*(fifo).buf) - 1, \
> > + .esize = sizeof(*(fifo).buf), \
> > + .data = __is_kfifo_ptr(&fifo) ? \
> > + NULL : \
> > + (fifo).buf, \
> > + } \
> > + } \
> > + }
> > +
> > +/*
> > + * INIT_KFIFO - Initialize a fifo declared by DECLARE_KFIFO
> > + * @fifo: name of the declared fifo datatype
> > + */
> > +#define INIT_KFIFO(fifo) fifo = __kfifo_initializer(fifo)
>
> Some versions of gcc generate quite poor code for
>
> struct foo f = { ... };
>
> It would generate an instance of the struct into the stack and then
> would memcpy it over, iirc. Please take a look, see if that's
> happening with this construct (might need to use an old gcc version)
> and if so, see if there's a fix?
>
> > +/*
> > + * DECLARE_KFIFO - macro to declare a fifo object
> > + * @fifo: name of the declared fifo
> > + * @type: type of the fifo elements
> > + * @size: the number of elements in the fifo, this must be a power of 2.
> > + */
> > +#define DECLARE_KFIFO(fifo, type, size) STRUCT_KFIFO(type, size) fifo
> > +
> > +/*
> > + * DEFINE_KFIFO - macro to define and initialize a fifo
> > + * @fifo: name of the declared fifo datatype
> > + * @type: type of the fifo elements
> > + * @size: the number of elements in the fifo, this must be a power of 2.
> > + *
> > + * Note: the macro can be used for global and local fifo data type variables
> > + */
> > +#define DEFINE_KFIFO(fifo, type, size) \
> > + DECLARE_KFIFO(fifo, type, size) = __kfifo_initializer(fifo)
> > +
> > +static inline unsigned int __must_check __kfifo_check(unsigned int val)
> > +{
> > + return val;
> > +}
>
> "check" is a poor term. Check what? For what? Unclear.
>
> If I knew what this was supposed to check, I could suggest a better
> name. But it's called "check", so I don't know :(
>
> > +/*
> > + * kfifo_initialized - Check if kfifo is initialized.
> > + * @fifo: fifo to check
> > + * Return %true if FIFO is initialized, otherwise %false.
> > + * Assumes the fifo was 0 before.
> > + *
> > + * Note: for in place fifo's this macro returns alway true
> > + */
> > +#define kfifo_initialized(fifo) ((fifo)->kfifo.data != NULL)
>
> kfifo_initialized() is suspicious. Any code which doesn't know whether
> its kfifo was initialised is probably confused, lame and broken.
> Fortunately kfifo_initialized() is not used anywhere. Please prepare a
> separate patch to kill it sometime.
>
> > +/*
> > + * kfifo_esize - returns the size of the element managed by the fifo
> > + * @fifo: the fifo to be used.
> > + */
> > +#define kfifo_esize(fifo) ((fifo)->kfifo.esize)
>
> This doesn't seem to be used anywhere either.
>
> > +/*
> > + * kfifo_recsize - returns the size of the record length field
> > + * @fifo: the fifo to be used.
> > + */
> > +#define kfifo_recsize(fifo) (sizeof(*(fifo)->rectype))
>
> Neither is this. What's going on?
>
> > +/*
> > + * kfifo_size - returns the size of the fifo in elements
> > + * @fifo: the fifo to be used.
> > + */
> > +#define kfifo_size(fifo) ((fifo)->kfifo.mask + 1)
> > +
> > +/*
> > + * kfifo_reset - removes the entire fifo content
> > + * @fifo: the fifo to be used.
> > + *
> > + * Note: usage of kfifo_reset() is dangerous. It should be only called when the
> > + * fifo is exclusived locked or when it is secured that no other thread is
> > + * accessing the fifo.
> > + */
> > +#define kfifo_reset(fifo) \
> > +(void)({ \
> > + typeof(fifo + 1) __tmp = (fifo); \
>
> What does the "+ 1" trick do?
>
> > + __tmp->kfifo.in = __tmp->kfifo.out = 0; \
> > +})
> > +
> >
> > ...
> >
> > +/*
> > + * kfifo_put - put data into the fifo
> > + * @fifo: the fifo to be used.
> > + * @val: the data to be added.
> > + *
> > + * This macro copies the given value into the fifo.
> > + *
> > + * Note that with only one concurrent reader and one concurrent
> > + * writer, you don't need extra locking to use these macro.
>
> There are quite a few typos in the comments in this patch.
>
> >
> > ...
> >
> > +/*
> > + * kfifo_out_locked - gets some data from the fifo using a spinlock for locking
> > + * @fifo: the fifo to be used.
> > + * @buf: pointer to the storage buffer
> > + * @n: max. number of elements to get
> > + * @lock: pointer to the spinlock to use for locking.
> > + *
> > + * This macro get the data from the fifo and return the numbers of elements
> > + * copied.
> > + */
> > +#define kfifo_out_locked(fifo, buf, n, lock) \
> > +__kfifo_check( \
> > +({ \
> > + unsigned long __flags; \
> > + unsigned int __ret; \
> > + spin_lock_irqsave(lock, __flags); \
> > + __ret = kfifo_out(fifo, buf, n); \
> > + spin_unlock_irqrestore(lock, __flags); \
> > + __ret; \
> > +}) \
> > +)
>
> This is poorly named. Generally "foo_locked" is to be called when the
> caller has already taken the lock. This identifier inverts that
> convention.
>
> > +/*
> > + * kfifo_from_user - puts some data from user space into the fifo
> > + * @fifo: the fifo to be used.
> > + * @from: pointer to the data to be added.
> > + * @len: the length of the data to be added.
> > + * @copied: pointer to output variable to store the number of copied bytes.
> > + *
> > + * This macro copies at most @len bytes from the @from into the
> > + * fifo, depending of the available space and returns -EFAULT/0
> > + *
> > + * Note that with only one concurrent reader and one concurrent
> > + * writer, you don't need extra locking to use these macro.
> > + */
> > +#define kfifo_from_user(fifo, from, len, copied) \
> > +__kfifo_check( \
> > +({ \
> > + typeof(fifo + 1) __tmp = (fifo); \
> > + const void __user *__from = (from); \
> > + unsigned int __len = (len); \
> > + unsigned int *__copied = (copied); \
> > + const size_t __recsize = sizeof(*__tmp->rectype); \
> > + struct __kfifo *__kfifo = &__tmp->kfifo; \
> > + (__recsize) ? \
> > + __kfifo_from_user_r(__kfifo, __from, __len, __copied, __recsize) : \
> > + __kfifo_from_user(__kfifo, __from, __len, __copied); \
> > +}) \
> > +)
> > +
> > +/*
> > + * kfifo_to_user - copies data from the fifo into user space
> > + * @fifo: the fifo to be used.
> > + * @to: where the data must be copied.
> > + * @len: the size of the destination buffer.
> > + * @copied: pointer to output variable to store the number of copied bytes.
> > + *
> > + * This macro copies at most @len bytes from the fifo into the
> > + * @to buffer and returns -EFAULT/0.
> > + *
> > + * Note that with only one concurrent reader and one concurrent
> > + * writer, you don't need extra locking to use these macro.
> > + */
> > +#define kfifo_to_user(fifo, to, len, copied) \
> > +__kfifo_check( \
> > +({ \
> > + typeof(fifo + 1) __tmp = (fifo); \
> > + void __user *__to = (to); \
> > + unsigned int __len = (len); \
> > + unsigned int *__copied = (copied); \
> > + const size_t __recsize = sizeof(*__tmp->rectype); \
> > + struct __kfifo *__kfifo = &__tmp->kfifo; \
> > + (__recsize) ? \
> > + __kfifo_to_user_r(__kfifo, __to, __len, __copied, __recsize) : \
> > + __kfifo_to_user(__kfifo, __to, __len, __copied); \
> > +}) \
> > +)
>
> Do these have any callers?
>
As a driver author, I have use for these functions. I've basically
implemented a scatterlist_from_user() function in my own driver, which
isn't upstream yet. See below for more.
> > +/*
> > + * kfifo_dma_in_prepare - setup a scatterlist for DMA input
> > + * @fifo: the fifo to be used.
> > + * @sgl: pointer to the scatterlist array.
> > + * @nents: number of entries in the scatterlist array
> > + * @len: number of elements to transfer.
> > + *
> > + * This macro fills a scatterlist for DMA input.
> > + * It returns the number entries in the scatterlist array
> > + *
> > + * Note that with only one concurrent reader and one concurrent
> > + * writer, you don't need extra locking to use these macros.
> > + */
> > +#define kfifo_dma_in_prepare(fifo, sgl, nents, len) \
> > +({ \
> > + typeof(fifo + 1) __tmp = (fifo); \
> > + struct scatterlist *__sgl = (sgl); \
> > + int __nents = (nents); \
> > + unsigned int __len = (len); \
> > + const size_t __recsize = sizeof(*__tmp->rectype); \
> > + struct __kfifo *__kfifo = &__tmp->kfifo; \
> > + (__recsize) ? \
> > + __kfifo_dma_in_prepare_r(__kfifo, __sgl, __nents, __len, __recsize) : \
> > + __kfifo_dma_in_prepare(__kfifo,__sgl, __nents, __len); \
> > +})
>
> Do the DMA functions have any callers? If not, why was all this stuff
> added?
>
I have use for these too. The driver in question needs to retrieve an
FPGA bitfile from userspace, setup a DMA, and trigger the (hardware)
FPGA programming logic. The programming logic toggles the DMA pins
appropriately until the FPGA bitfile is loaded into the FPGA.
The driver isn't upstream, since it is only useful on my board, and no
other hardware in existence. I didn't think anyone would care. This
would remove my needs for a custom FIFO-ish type that I've invented on
top of a scatterlist, all so I could use dma_map_sg().
I have another driver which periodically reads (via DMA) from the FPGA
into a scatterlist. These scatterlists are buffered up until userspace
reads them via a char device. The driver essentially goes:
preallocate SG's
interrupt -> DMA to SG -> SG to userspace
I had to roll my own implementation to keep track of how much data had
been copied to userspace out of the buffer. Speaking as a driver author,
the new kfifo userspace and DMA API's would make my life just that much
easier.
Ira
> >
> > ...
> >
> > --- linux-2.6.33-rc5.dummy/kernel/kfifo.c 1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.33-rc5.new/kernel/kfifo.c 2010-01-27 13:33:56.456825816 +0100
> > @@ -0,0 +1,583 @@
> > +/*
> > + * A generic kernel FIFO implementation
> > + *
> > + * Copyright (C) 2009/2010 Stefani Seibold <stefani@seibold.net>
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/log2.h>
> > +#include <linux/uaccess.h>
> > +
> > +static unsigned int roundup_diff(unsigned int val, unsigned int size)
> > +{
> > + if (size <= 1)
> > + return size;
> > + return (val + size - 1) / size;
> > +}
>
> Would be useful to add a comment explaining why this exists.
>
> Can we not use existing general facilities?
>
> Should this be added to the existing general facilities?
>
> > +static inline unsigned int kfifo_unused(struct __kfifo *fifo)
> > +{
> > + return (fifo->mask + 1) - (fifo->in - fifo->out);
> > +}
>
> hm, how does this differ from kfifo_avail()?
>
> Should this use existing helpers such as kfifo_avail(), kfifo_len(),
> etc?
>
> > +int __kfifo_alloc(struct __kfifo *fifo, unsigned int size,
> > + size_t esize, gfp_t gfp_mask)
> > +{
> > + /*
> > + * round up to the next power of 2, since our 'let the indices
> > + * wrap' technique works only in this case.
> > + */
> > + if (!is_power_of_2(size))
> > + size = rounddown_pow_of_two(size);
> > +
> > + if (size < 2)
> > + return -EINVAL;
> > +
> > + fifo->in = fifo->out = 0;
>
> fifo->in = 0;
> fifo->out = 0;
>
> is usually preferred.
>
> > + fifo->data = kmalloc(size * esize, gfp_mask);
> > +
> > + if (!fifo->data) {
> > + fifo->mask = 0;
> > + return -ENOMEM;
> > + }
> > +
> > + fifo->mask = size - 1;
> > + fifo->esize = esize;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(__kfifo_alloc);
> > +
> > +void __kfifo_free(struct __kfifo *fifo)
> > +{
> > + kfree(fifo);
> > + fifo->data = 0;
> > + fifo->mask = 0;
> > + fifo->in = fifo->out = 0;
> > +}
> > +EXPORT_SYMBOL(__kfifo_free);
>
> err, no. This scribbles on memory after having returned it to slab!
>
> Which implies that this code wasn't tested with suitable kernel
> debugging options enabled. Please absorb
> Documentation/SubmitChecklist.
>
> Initialising memory to zero just prior to freeing it is lame anyway.
> And it can hide bugs. Just remove this and return the fifo to slab
> as-is.
>
> > +
> > +int __kfifo_init(struct __kfifo *fifo, void *buffer,
> > + unsigned int size, size_t esize)
> > +{
> > + size /= esize;
> > +
> > + if (size)
> > + size = rounddown_pow_of_two(size);
> > +
> > + if (size < 2)
> > + return -EINVAL;
> > +
> > + fifo->data = buffer;
> > + fifo->in = fifo->out = 0;
> > + fifo->mask = size - 1;
> > + fifo->esize = esize;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(__kfifo_init);
> > +
> > +static void kfifo_copy_in(struct __kfifo *fifo, const void *src,
> > + unsigned int len, unsigned int off)
> > +{
> > + unsigned int size = fifo->mask + 1;
> > + unsigned int esize = fifo->esize;
> > + unsigned int l;
> > +
> > + off &= fifo->mask;
> > + if (esize != 1) {
> > + off *= esize;
> > + size *= esize;
> > + len *= esize;
> > + }
> > + l = min(len, size - off);
> > +
> > + memcpy(fifo->data + off, src, l);
> > + memcpy(fifo->data, src + l, len - l);
> > + smp_wmb();
>
> It would be better if all the barriers in this code had comments
> explaining why they're there. Looking at the above code it's quite
> hard to determine which other code the barrier is protecting the data
> against.
>
> > +}
> > +
> >
> > ...
> >
> > +static unsigned long kfifo_copy_from_user(struct __kfifo *fifo,
> > + const void *src, unsigned int len, unsigned int off,
> > + unsigned int *copied)
> > +{
> > + unsigned int size = fifo->mask + 1;
> > + unsigned int esize = fifo->esize;
> > + unsigned int l;
> > + unsigned long ret;
> > +
> > + off &= fifo->mask;
> > + if (esize != 1) {
> > + off *= esize;
> > + size *= esize;
> > + len *= esize;
> > + }
> > + l = min(len, size - off);
> > +
> > + ret = copy_from_user(fifo->data + off, src, l);
> > + if (unlikely(ret))
> > + ret = roundup_diff(ret + len - l, esize);
> > + else {
> > + ret = copy_from_user(fifo->data, src + l, len - l);
> > + if (unlikely(ret))
> > + ret = roundup_diff(ret, esize);
> > + }
> > + smp_wmb();
> > + if (copied)
>
> Can `copied' ever be NULL? If not, remove this test.
>
> > + *copied = len - ret;
> > + return ret;
> > +}
>
> All pointers which refer to userspace should have the __user tag. And
> the code should be checked with sparse, please.
>
> It's rather hard to work out the meaning of this function's return value.
>
> > +int __kfifo_from_user(struct __kfifo *fifo, const void __user *from,
> > + unsigned long len, unsigned int *copied)
> > +{
> > + unsigned int l;
> > + unsigned long ret;
> > + unsigned int esize = fifo->esize;
> > + int err;
> > +
> > + if (esize != 1)
> > + len /= esize;
> > +
> > + l = kfifo_unused(fifo);
> > + if (len > l)
> > + len = l;
> > +
> > + ret = kfifo_copy_from_user(fifo, from, len, fifo->in, copied);
> > + if (unlikely(ret)) {
> > + len -= ret;
> > + err = -EFAULT;
> > + }
> > + else
> > + err = 0;
> > + fifo->in += len;
> > + return err;
> > +}
> > +EXPORT_SYMBOL(__kfifo_from_user);
> > +
> >
> > ...
> >
> > +static int setup_sgl_buf(struct scatterlist *sgl, void *buf,
> > + int nents, unsigned int len)
> > +{
> > + int n;
> > + unsigned int l;
> > + unsigned int off;
> > + struct page *page;
> > +
> > + if (!nents)
> > + return 0;
> > +
> > + if (!len)
> > + return 0;
> > +
> > + n = 0;
> > + page = virt_to_page(buf);
> > + off = offset_in_page(buf);
> > + l = 0;
> > +
> > + while(len >= l + PAGE_SIZE - off) {
> > + struct page *npage;
> > +
> > + l += PAGE_SIZE;
> > + buf += PAGE_SIZE;
> > + npage = virt_to_page(buf);
> > + if (page_to_phys(page) != page_to_phys(npage) - l) {
> > + sgl->page_link = 0;
> > + sg_set_page(sgl++, page, l - off, off);
> > + if (++n == nents)
> > + return n;
> > + page = npage;
> > + len -= l - off;
> > + l = off = 0;
> > + }
> > + }
> > + sgl->page_link = 0;
> > + sg_set_page(sgl++, page, len, off);
> > + return n + 1;
> > +}
>
> Again, I just don't see why all this sg and dma stuff is here. Has it
> been tested?
>
> >
> > ...
> >
> > +/**
> > + * __kfifo_peek_n internal helper function for determinate the length of
> > + * the next record in the fifo
> > + */
>
> This comment purports to be kerneldoc ("/**") but it isn't.
>
> > +static unsigned int __kfifo_peek_n(struct __kfifo *fifo, size_t recsize)
> > +{
> > + unsigned int l;
> > + unsigned int mask = fifo->mask;
> > + unsigned char *data = fifo->data;
> > +
> > + l = __KFIFO_PEEK(data, fifo->out, mask);
> > +
> > + if (--recsize)
> > + l |= __KFIFO_PEEK(data, fifo->out + 1, mask) << 8;
> > +
> > + return l;
> > +}
> > +
> > +#define __KFIFO_POKE(data, in, mask, val) \
> > + ( \
> > + (data)[(in) & (mask)] = (unsigned char)(val) \
> > + )
> > +
> > +/**
> > + * __kfifo_poke_n internal helper function for storeing the length of
> > + * the record into the fifo
> > + */
>
> Please check all kerneldoc stuff.
>
> >
> > ...
> >
>
--
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/
==============================================================================
TOPIC: syslog: use defined constants instead of raw numbers
http://groups.google.com/group/linux.kernel/t/e1594790606b7db6?hl=en
==============================================================================
== 1 of 2 ==
Date: Wed, Feb 3 2010 12:50 pm
From: "Serge E. Hallyn"
Quoting Kees Cook (kees.cook@canonical.com):
> Right now the syslog "type" actions are just raw numbers which makes
> the source difficult to follow. This patch replaces the raw numbers
> with defined constants for some level of sanity.
>
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
yes please!
Acked-by: Serge Hallyn <serue@us.ibm.com>
> ---
> fs/proc/kmsg.c | 11 ++++++-----
> include/linux/syslog.h | 23 +++++++++++++++++++++++
> kernel/printk.c | 45 +++++++++++++++++++--------------------------
> security/commoncap.c | 5 +++--
> security/selinux/hooks.c | 21 +++++++++++----------
> 5 files changed, 62 insertions(+), 43 deletions(-)
>
> diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
> index 4a08999..1ac9d2b 100644
> --- a/fs/proc/kmsg.c
> +++ b/fs/proc/kmsg.c
> @@ -21,27 +21,28 @@ extern wait_queue_head_t log_wait;
>
> static int kmsg_open(struct inode * inode, struct file * file)
> {
> - return do_syslog(1, NULL, 0, 1);
> + return do_syslog(SYSLOG_ACTION_OPEN, NULL, 0, 1);
> }
>
> static int kmsg_release(struct inode * inode, struct file * file)
> {
> - (void) do_syslog(0, NULL, 0, 1);
> + (void) do_syslog(SYSLOG_ACTION_CLOSE, NULL, 0, 1);
> return 0;
> }
>
> static ssize_t kmsg_read(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
> {
> - if ((file->f_flags & O_NONBLOCK) && !do_syslog(9, NULL, 0, 1))
> + if ((file->f_flags & O_NONBLOCK) &&
> + !do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, 1))
> return -EAGAIN;
> - return do_syslog(2, buf, count, 1);
> + return do_syslog(SYSLOG_ACTION_READ, buf, count, 1);
> }
>
> static unsigned int kmsg_poll(struct file *file, poll_table *wait)
> {
> poll_wait(file, &log_wait, wait);
> - if (do_syslog(9, NULL, 0, 1))
> + if (do_syslog(SYSLOG_ACTION_SIZE_UNREAD, NULL, 0, 1))
> return POLLIN | POLLRDNORM;
> return 0;
> }
> diff --git a/include/linux/syslog.h b/include/linux/syslog.h
> index f8c9d94..8c2c422 100644
> --- a/include/linux/syslog.h
> +++ b/include/linux/syslog.h
> @@ -21,6 +21,29 @@
> #ifndef _LINUX_SYSLOG_H
> #define _LINUX_SYSLOG_H
>
> +/* Close the log. Currently a NOP. */
> +#define SYSLOG_ACTION_CLOSE 0
> +/* Open the log. Currently a NOP. */
> +#define SYSLOG_ACTION_OPEN 1
> +/* Read from the log. */
> +#define SYSLOG_ACTION_READ 2
> +/* Read all messages remaining in the ring buffer. */
> +#define SYSLOG_ACTION_READ_ALL 3
> +/* Read and clear all messages remaining in the ring buffer */
> +#define SYSLOG_ACTION_READ_CLEAR 4
> +/* Clear ring buffer. */
> +#define SYSLOG_ACTION_CLEAR 5
> +/* Disable printk's to console */
> +#define SYSLOG_ACTION_CONSOLE_OFF 6
> +/* Enable printk's to console */
> +#define SYSLOG_ACTION_CONSOLE_ON 7
> +/* Set level of messages printed to console */
> +#define SYSLOG_ACTION_CONSOLE_LEVEL 8
> +/* Return number of unread characters in the log buffer */
> +#define SYSLOG_ACTION_SIZE_UNREAD 9
> +/* Return size of the log buffer */
> +#define SYSLOG_ACTION_SIZE_BUFFER 10
> +
> int do_syslog(int type, char __user *buf, int count, int from_file);
>
>
No comments:
Post a Comment