Re: [PATCH v1 00/11] locks: scalability improvements for file locking
Jeff Layton wrote: > Might be nice to look at some profiles to confirm all of that. I'd also > be curious how much variation there was in the results above, as they're > pretty close. > The above is just a random representative sample. The results are pretty close when running this test, but I can average up several runs and present the numbers. I plan to get a bare-metal test box on which to run some more detailed testing and maybe some profiling this week. Just contributing more runs into the mean doesn't tell us anything about the variance. With numbers that close you need the variance to tell whether it's a significant change. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 00/11] locks: scalability improvements for file locking
Jeff Layton wrote: Might be nice to look at some profiles to confirm all of that. I'd also be curious how much variation there was in the results above, as they're pretty close. The above is just a random representative sample. The results are pretty close when running this test, but I can average up several runs and present the numbers. I plan to get a bare-metal test box on which to run some more detailed testing and maybe some profiling this week. Just contributing more runs into the mean doesn't tell us anything about the variance. With numbers that close you need the variance to tell whether it's a significant change. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/16] hashtable: introduce a small and naive hashtable
Sasha Levin wrote:
On Tue, Oct 30, 2012 at 5:42 PM, Tejun Heo wrote:
> Hello,
>
> Just some nitpicks.
>
> On Tue, Oct 30, 2012 at 02:45:57PM -0400, Sasha Levin wrote:
>> +/* Use hash_32 when possible to allow for fast 32bit hashing in 64bit
kernels. */
>> +#define hash_min(val, bits)
\
>> +({
\
>> + sizeof(val) <= 4 ?
\
>> + hash_32(val, bits) :
\
>> + hash_long(val, bits);
\
>> +})
>
> Doesn't the above fit in 80 column. Why is it broken into multiple
> lines? Also, you probably want () around at least @val. In general,
> it's a good idea to add () around any macro argument to avoid nasty
> surprises.
It was broken to multiple lines because it looks nicer that way (IMO).
If we wrap it with () it's going to go over 80, so it's going to stay
broken down either way :)
I would prefer the body be all on one line too. But shouldn't this be a
static inline function?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 01/16] hashtable: introduce a small and naive hashtable
Sasha Levin wrote:
On Tue, Oct 30, 2012 at 5:42 PM, Tejun Heo t...@kernel.org wrote:
Hello,
Just some nitpicks.
On Tue, Oct 30, 2012 at 02:45:57PM -0400, Sasha Levin wrote:
+/* Use hash_32 when possible to allow for fast 32bit hashing in 64bit
kernels. */
+#define hash_min(val, bits)
\
+({
\
+ sizeof(val) = 4 ?
\
+ hash_32(val, bits) :
\
+ hash_long(val, bits);
\
+})
Doesn't the above fit in 80 column. Why is it broken into multiple
lines? Also, you probably want () around at least @val. In general,
it's a good idea to add () around any macro argument to avoid nasty
surprises.
It was broken to multiple lines because it looks nicer that way (IMO).
If we wrap it with () it's going to go over 80, so it's going to stay
broken down either way :)
I would prefer the body be all on one line too. But shouldn't this be a
static inline function?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Apparent serious progressive ext4 data corruption bug in 3.6.3 (and other stable branches?)
Theodore Ts'o wrote: The problem is this code isn't done yet, and journal_checksum is really not ready for prime time. When it is ready, my plan is to wire it up so it is enabled by default; at the moment, it was intended for developer experimentation only. As I said, it's my fault for not clearly labelling it "Not for you!", or putting it under an #ifdef to prevent unwary civilians from coming across the feature and saying, "oooh, shiny!" and turning it on. :-( Perhaps a word or two in the mount man page would be appropriate? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Apparent serious progressive ext4 data corruption bug in 3.6.3 (and other stable branches?)
Theodore Ts'o wrote: The problem is this code isn't done yet, and journal_checksum is really not ready for prime time. When it is ready, my plan is to wire it up so it is enabled by default; at the moment, it was intended for developer experimentation only. As I said, it's my fault for not clearly labelling it Not for you!, or putting it under an #ifdef to prevent unwary civilians from coming across the feature and saying, oooh, shiny! and turning it on. :-( Perhaps a word or two in the mount man page would be appropriate? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Jan Engelhardt wrote: Your way does not function as originally desired. * base10len(i) no longer expands to a compile-time constant * you will definitely have a variable base10len_vals in your objects, so you waste a read operation whenever it is used. No, I meant my original way: #define base10len(i) (sizeof(i) * 8 * 3 / 10 + 1) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Bernd Petrovitsch wrote: [] > Pure K: We can (and should) make it "const" too. No "const" in K either. But I think we can assume the kernel will be compiled with something a bit more advance. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Jan Engelhardt wrote: >A pure K version would use a string: >#define base10len(i) "\0x1\0x3\0x5\0x8\0x0A\0x0D\0x0F\0x11\0x14"[sizeof(i)] >(if I converted them properly into hexadecimal) The syntax is \x01\x03\x05... K doesn't have the \x escape, only \0 (octal). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Bernd Petrovitsch wrote:
On Mon, 2012-09-10 at 08:19 +0200, Jan Engelhardt wrote:
> On Tuesday 2012-08-21 23:29, J. Bruce Fields wrote:
[...]
> >+/*
> >+ * length of the decimal representation of an unsigned integer. Just an
> >+ * approximation, but it's right for types of size 1 to 36 bytes:
> >+ */
> >+#define base10len(i) (sizeof(i) * 24 / 10 + 1)
>
> gcc provides... "interesting" features at times.
>
> /* for unsigned "i"s */
> #define base10len(i) ((const int[]){1,3,5,8,10,13,15,17,20}[i])
Shouldn't that have been
snip
#define base10len(i) ((const int[]){1,3,5,8,10,13,15,17,20}[sizeof(i)])
snip
?
A pure K version would use a string:
snip
#define base10len(i) "\0x1\0x3\0x5\0x8\0x0A\0x0D\0x0F\0x11\0x14"[sizeof(i)]
snip
(if I converted them properly into hexadecimal) and that gives a "char"
which is happily promoted to whatever one needs in that place.
1. That may give you a signed char on some architectures, which is not what
you want (although it doesn't matter since the values are all < 128)
2. If you put this in a .h, you'll get multiple copies of the array
3. No bounds checking (but in ninja K style you never check bounds)
4. Unreadable.
Pure K:
base10.h:
extern unsigned char base10len_vals[];
#define base10len(i) (base10len_vals[sizeof(i)])
base10.c:
unsigned char base10len_vals[] = {1,3,5,8,10,13,15,17,20};
But I still like my way better.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Bernd Petrovitsch wrote:
On Mon, 2012-09-10 at 08:19 +0200, Jan Engelhardt wrote:
On Tuesday 2012-08-21 23:29, J. Bruce Fields wrote:
[...]
+/*
+ * length of the decimal representation of an unsigned integer. Just an
+ * approximation, but it's right for types of size 1 to 36 bytes:
+ */
+#define base10len(i) (sizeof(i) * 24 / 10 + 1)
gcc provides... interesting features at times.
/* for unsigned is */
#define base10len(i) ((const int[]){1,3,5,8,10,13,15,17,20}[i])
Shouldn't that have been
snip
#define base10len(i) ((const int[]){1,3,5,8,10,13,15,17,20}[sizeof(i)])
snip
?
A pure KR-C version would use a string:
snip
#define base10len(i) \0x1\0x3\0x5\0x8\0x0A\0x0D\0x0F\0x11\0x14[sizeof(i)]
snip
(if I converted them properly into hexadecimal) and that gives a char
which is happily promoted to whatever one needs in that place.
1. That may give you a signed char on some architectures, which is not what
you want (although it doesn't matter since the values are all 128)
2. If you put this in a .h, you'll get multiple copies of the array
3. No bounds checking (but in ninja KR style you never check bounds)
4. Unreadable.
Pure KR:
base10.h:
extern unsigned char base10len_vals[];
#define base10len(i) (base10len_vals[sizeof(i)])
base10.c:
unsigned char base10len_vals[] = {1,3,5,8,10,13,15,17,20};
But I still like my way better.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Jan Engelhardt wrote: A pure KR-C version would use a string: #define base10len(i) \0x1\0x3\0x5\0x8\0x0A\0x0D\0x0F\0x11\0x14[sizeof(i)] (if I converted them properly into hexadecimal) The syntax is \x01\x03\x05... KR doesn't have the \x escape, only \0 (octal). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Bernd Petrovitsch wrote: [] Pure KR: We can (and should) make it const too. No const in KR either. But I think we can assume the kernel will be compiled with something a bit more advance. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Jan Engelhardt wrote: Your way does not function as originally desired. * base10len(i) no longer expands to a compile-time constant * you will definitely have a variable base10len_vals in your objects, so you waste a read operation whenever it is used. No, I meant my original way: #define base10len(i) (sizeof(i) * 8 * 3 / 10 + 1) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Al Viro wrote: On Tue, Aug 21, 2012 at 05:22:27PM -0400, Jim Rees wrote: > J. Bruce Fields wrote: > > From: "J. Bruce Fields" > > I've seen a couple examples recently where we've gotten this wrong. > Maybe something like this would help? Is there some better way? > > (Approximation due to Jim Rees). > > Please add Suggested-by: Jim Rees . I'm thinking of > patenting the algorithm. Is that a joke? Yes, that's a joke. Sorry if it wasn't obvious. I am, however offering up licenses at a cost of one pint of beer per thousand digits converted. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
J. Bruce Fields wrote: From: "J. Bruce Fields" I've seen a couple examples recently where we've gotten this wrong. Maybe something like this would help? Is there some better way? (Approximation due to Jim Rees). Please add Suggested-by: Jim Rees . I'm thinking of patenting the algorithm. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
J. Bruce Fields wrote: From: J. Bruce Fields bfie...@redhat.com I've seen a couple examples recently where we've gotten this wrong. Maybe something like this would help? Is there some better way? (Approximation due to Jim Rees). Please add Suggested-by: Jim Rees r...@umich.edu. I'm thinking of patenting the algorithm. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] strings: helper for maximum decimal encoding of an unsigned integer
Al Viro wrote: On Tue, Aug 21, 2012 at 05:22:27PM -0400, Jim Rees wrote: J. Bruce Fields wrote: From: J. Bruce Fields bfie...@redhat.com I've seen a couple examples recently where we've gotten this wrong. Maybe something like this would help? Is there some better way? (Approximation due to Jim Rees). Please add Suggested-by: Jim Rees r...@umich.edu. I'm thinking of patenting the algorithm. Is that a joke? Yes, that's a joke. Sorry if it wasn't obvious. I am, however offering up licenses at a cost of one pint of beer per thousand digits converted. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
Sasha Levin wrote: > Learning from what happened in this specific case, there are actually 2 issues here: > > - Array size was constant and too small, which is solved by the patch above. > - We were blindly trying to sprintf() into that array, this issue may pop back up if someone decides to change the format string forgetting to modify the array declaration. > The original patch changed the sprintf to snprintf, and that still seems like a good idea. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
Dave Jones wrote: On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote: > You could use something like: > > char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */ > > since there are roughly 10 bits for every 3 decimal digits. > > But I'm obviously confused, because I don't understand why tbuf needs to be > any more than 10 + 2. Unsigned long isn't necessarily 32 bits. On 64-bit systems %lu can be up to 18446744073709551615 Thanks. You caught me thinking "Intel." How embarrassing. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
J. Bruce Fields wrote:
On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
> The buffer size in read_flush() is too small for the longest possible values
> for it. This can lead to a kernel stack corruption:
Thanks!
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 2afd2a8..f86d95e 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char
__user *buf,
> size_t count, loff_t *ppos,
> struct cache_detail *cd)
> {
> - char tbuf[20];
> + char tbuf[22];
I wonder how common this sort of calculation is in the kernel? It might
provide some peace of mind to be able to write this something like
char tbuf[MAXLEN_BASE10_UL + 2] /* + 2 for final "\n\0" */
You could use something like:
char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final
"\n\0" */
since there are roughly 10 bits for every 3 decimal digits.
But I'm obviously confused, because I don't understand why tbuf needs to be
any more than 10 + 2.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
J. Bruce Fields wrote:
On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
The buffer size in read_flush() is too small for the longest possible values
for it. This can lead to a kernel stack corruption:
Thanks!
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 2afd2a8..f86d95e 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char
__user *buf,
size_t count, loff_t *ppos,
struct cache_detail *cd)
{
- char tbuf[20];
+ char tbuf[22];
I wonder how common this sort of calculation is in the kernel? It might
provide some peace of mind to be able to write this something like
char tbuf[MAXLEN_BASE10_UL + 2] /* + 2 for final \n\0 */
You could use something like:
char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final
\n\0 */
since there are roughly 10 bits for every 3 decimal digits.
But I'm obviously confused, because I don't understand why tbuf needs to be
any more than 10 + 2.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
Dave Jones wrote: On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote: You could use something like: char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final \n\0 */ since there are roughly 10 bits for every 3 decimal digits. But I'm obviously confused, because I don't understand why tbuf needs to be any more than 10 + 2. Unsigned long isn't necessarily 32 bits. On 64-bit systems %lu can be up to 18446744073709551615 Thanks. You caught me thinking Intel. How embarrassing. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
Sasha Levin wrote: Learning from what happened in this specific case, there are actually 2 issues here: - Array size was constant and too small, which is solved by the patch above. - We were blindly trying to sprintf() into that array, this issue may pop back up if someone decides to change the format string forgetting to modify the array declaration. The original patch changed the sprintf to snprintf, and that still seems like a good idea. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
