Mailing List Archive: Linux: Kernel

[GIT PULL] percpu fixes for 2.6.32-rc6

1 2

View All

Index | Next | Previous | View Threaded

tj at kernel

Nov 9, 2009, 10:04 PM

Post #1 of 29 (626 views)
Permalink

[GIT PULL] percpu fixes for 2.6.32-rc6

Hello, Linus.

Please pull from the following percpu fix branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git for-linus

It fixes a possible deadlock caused by lock ordering inversion through
irq.

Thanks.
---
Tejun Heo (1):
percpu: fix possible deadlock via irq lock inversion

mm/percpu.c | 17 +++++++++++++++--

diff --git a/mm/percpu.c b/mm/percpu.c
index d907971..30cd343 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -372,7 +372,7 @@ static struct pcpu_chunk *pcpu_chunk_addr_search(void *addr)
static int pcpu_extend_area_map(struct pcpu_chunk *chunk, unsigned long *flags)
{
int new_alloc;
- int *new;
+ int *new, *old = NULL;
size_t size;

/* has enough? */
@@ -407,10 +407,23 @@ static int pcpu_extend_area_map(struct pcpu_chunk *chunk, unsigned long *flags)
* one of the first chunks and still using static map.
*/
if (chunk->map_alloc >= PCPU_DFL_MAP_ALLOC)
- pcpu_mem_free(chunk->map, size);
+ old = chunk->map;

chunk->map_alloc = new_alloc;
chunk->map = new;
+
+ /*
+ * pcpu_mem_free() might end up calling vfree() which uses
+ * IRQ-unsafe lock and thus can't be called with pcpu_lock
+ * held. Release and reacquire pcpu_lock if old map needs to
+ * be freed.
+ */
+ if (old) {
+ spin_unlock_irqrestore(&pcpu_lock, *flags);
+ pcpu_mem_free(old, size);
+ spin_lock_irqsave(&pcpu_lock, *flags);
+ }
+
return 0;
}

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Nov 10, 2009, 9:10 AM

Post #2 of 29 (609 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

On Tue, 10 Nov 2009, Tejun Heo wrote:
>
> Please pull from the following percpu fix branch.

No way in hell.

> It fixes a possible deadlock caused by lock ordering inversion through
> irq.

.. and it does so by introducing a new bug. No thank you.

> +
> + /*
> + * pcpu_mem_free() might end up calling vfree() which uses
> + * IRQ-unsafe lock and thus can't be called with pcpu_lock
> + * held. Release and reacquire pcpu_lock if old map needs to
> + * be freed.
> + */
> + if (old) {
> + spin_unlock_irqrestore(&pcpu_lock, *flags);
> + pcpu_mem_free(old, size);
> + spin_lock_irqsave(&pcpu_lock, *flags);
> + }

Routines that drop and then re-take the lock should be banned, as it's
almost always a bug waiting to happen. As it is this time:

> return 0;

Now the caller will happily continue to traverse a list that may no longer
be valid, because you dropped the lock.

Really. This thing is total sh*t. It was misdesigned to start with, and
the calling convention is wrong. That 'pcpu_extend_area_map()' function
should be split up into two functions: 'pcpu_needs_to_extend()' that never
drops the lock, and 'pcpu_extend_area()' that _always_ drops the lock
(and then returns an error if it can't allocate the memory).

Not that shit-for-brains that may or may not drop the lock, and then
returns an incorrect error return depending on whether it did.

In other words: fix the sh*t, don't add even more to it. That 'return 0'
was and is wrong. It should have been a 'return 1'. And thank the Gods
that I looked at it,

Sure, you can fix the bug by just returning 1. But you can't fix the total
crap of a calling convention that way. Fix it properly as outlined above,
and remember: functions that drop locks that were held when called are
EVIL and almost always the source of really subtle races.

As it was in this case.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 10, 2009, 10:33 AM

Post #3 of 29 (604 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello, Linus.

Linus Torvalds wrote:
>> + if (old) {
>> + spin_unlock_irqrestore(&pcpu_lock, *flags);
>> + pcpu_mem_free(old, size);
>> + spin_lock_irqsave(&pcpu_lock, *flags);
>> + }
>
> Routines that drop and then re-take the lock should be banned, as it's
> almost always a bug waiting to happen. As it is this time:

Yeap, they usually are. In this case, it's a little bit different.
There are two locks - pcpu_alloc_mutex and pcpu_lock.
pcpu_alloc_mutex protects the whole alloc and reclaim paths while
pcpu_lock protects the part used by free path - area maps in chunks
and pcpu_slot array pointing to chunks.

So, under pcpu_alloc_mutex which pcpu_extend_area_map() is called with
and never drops, the chunk never goes away nor can anything be
allocated from it. Dropping pcpu_lock only allows free path to come
inbetween and mark areas in the area map of the chunk and maybe
relocate the chunk to another pcpu_slot according to new free area
size - both operations should be safe. IOW, it's not really dropping
the main lock here.

At first, both alloc and free paths were protected by single mutex.
The original percpu allocator always assumed GFP_KERNEL allocation, so
I thought that should do it. Unfortunately, I was forgetting about
the free path which was allowed to be called from atomic context, so
the lock was split into two so that the free path can be called from
atomic context.

The reason why this somewhat convoluted locking was chosen over making
both alloc and free paths irq-safe was partly because it was easier
but mostly because percpu allocator's dependence on vmalloc allocator.
All vmalloc area related lockings assume not to be called from irq
context which goes back to having to make cross cpu invalidate calls,
which make it difficult to make percpu allocator irqsafe as a whole.
So, the locking impedance matching was done in the percpu free path
and the temporary inner lock droppings in pcpu_extend_are_map() are
the byproducts.

Christoph Lameter has been saying that atomic percpu allocations would
be beneficial for certain callers. Pushing the problem down to the
vmalloc layer where the problem originates and making percpu locking
more usual would be nice too. I'm still not sure whether it would
justify the added complexity (it will probably require putting vmalloc
reclamation path to a workqueue) but that could be the ultimate right
thing to do here.

If I'm missing something, I'm sure you'll hammer it into me.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Nov 10, 2009, 10:54 AM

Post #4 of 29 (602 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

On Wed, 11 Nov 2009, Tejun Heo wrote:
>
> If I'm missing something, I'm sure you'll hammer it into me.

Here's from the comments on that function:

* RETURNS:
* 0 if noop, 1 if successfully extended, -errno on failure.

and here's from one of the main callers:

list_for_each_entry(chunk, &pcpu_slot[slot], list) {
...
switch (pcpu_extend_area_map(chunk, &flags)) {
case 0:
break;
case 1:
goto restart; /* pcpu_lock dropped, restart */

where that '&pcpu_slot[slot]' list is protected by the pcpu_lock, and
nothing else. At least according to all the _other_ comments in that file.
Including the one that very much tries to _explain_ the locking at the
top, quote:

"The latter is a spinlock and protects the index data structures - chunk
slots, chunks and area maps in chunks."

So as far as I can tell, either the comments are all crap, the whole
restart code is pointless and in fact the whole spin-lock is seemingly
almost entirely pointless to begin with (since pcpu_alloc_mutex is the
only thing that matters), or the code is buggy.

Also, quite frankly, even if the code _isn't_ buggy, it's still wrong to
release a lock that somebody else took. It's a sure-fire way to write
unmaintainable code with bugs almost guaranteed in the future. Yes, it
happens, and sometimes it's the only sane way to do it, but in this case
that really isn't true as far as I can tell.

From my (admittedly fairly quick) look, my suggested split-up really would
make the code _more_ readable (no need for that subtle "negative, zero or
positive all mean different things" logic), and hopefully avoid the whole
"drop the lock that somebody else took", because we could drop it in the
caller where it was taken.

So it all boils down to: the code is unquestionably ugly and almost
certainly broken. And if it isn't broken, then _all_ the comments are
total crap.

Your choice.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 10, 2009, 11:25 AM

Post #5 of 29 (609 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello,

Linus Torvalds wrote:
> On Wed, 11 Nov 2009, Tejun Heo wrote:
>> If I'm missing something, I'm sure you'll hammer it into me.
>
> Here's from the comments on that function:
>
> * RETURNS:
> * 0 if noop, 1 if successfully extended, -errno on failure.
>
> and here's from one of the main callers:
>
> list_for_each_entry(chunk, &pcpu_slot[slot], list) {
> ...
> switch (pcpu_extend_area_map(chunk, &flags)) {
> case 0:
> break;
> case 1:
> goto restart; /* pcpu_lock dropped, restart */
>
> where that '&pcpu_slot[slot]' list is protected by the pcpu_lock, and
> nothing else. At least according to all the _other_ comments in that file.
> Including the one that very much tries to _explain_ the locking at the
> top, quote:

Oh, yeah, right. I was too fixated on the part modified by the patch.

> "The latter is a spinlock and protects the index data structures - chunk
> slots, chunks and area maps in chunks."
>
> So as far as I can tell, either the comments are all crap, the whole
> restart code is pointless and in fact the whole spin-lock is seemingly
> almost entirely pointless to begin with (since pcpu_alloc_mutex is the
> only thing that matters), or the code is buggy.

The return value is wrong but it wouldn't lead to oops. There's a
very slight chance that it might end up creating extra chunk when not
necessary - probably why it went unnoticed all this time. The
spin-lock is only to allow free_percpu() to be called from atomic
context, so its usefulness would only be visible if you look at
free_percpu() too.

> Also, quite frankly, even if the code _isn't_ buggy, it's still wrong to
> release a lock that somebody else took. It's a sure-fire way to write
> unmaintainable code with bugs almost guaranteed in the future. Yes, it
> happens, and sometimes it's the only sane way to do it, but in this case
> that really isn't true as far as I can tell.
>
> From my (admittedly fairly quick) look, my suggested split-up really would
> make the code _more_ readable (no need for that subtle "negative, zero or
> positive all mean different things" logic), and hopefully avoid the whole
> "drop the lock that somebody else took", because we could drop it in the
> caller where it was taken.
>
> So it all boils down to: the code is unquestionably ugly and almost
> certainly broken. And if it isn't broken, then _all_ the comments are
> total crap.

Yeap, the return value definitely is broken and the rather ugly
calling convention is remanant from the days when there was only
single mutex protecting the whole thing.

I think this type of function is a bit special in locking requirement
tho. The initial step - checking whether the operation is necessary -
requires lock and the final step - copying over to the new thing and
installing it - also requires the lock, so unless there's one
unnecessary unlock/lock pair, the second function would be called
without lock but return with lock, which probably is safer than
releasing and regrabbing lock in the middle but still not quite
pretty.

In this case, as the second function needs to release to free the old
map, the extra unlock/lock pair is actually necessary. Splitting into
two functions is fine but I think it would be better to fix it first
and then split them in following patches so that it can be bisected if
I screw up while splitting, right?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 10, 2009, 11:37 AM

Post #6 of 29 (602 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> In this case, as the second function needs to release to free the old
> map, the extra unlock/lock pair is actually necessary. Splitting into
> two functions is fine but I think it would be better to fix it first
> and then split them in following patches so that it can be bisected if
> I screw up while splitting, right?

i think Linus's point was that this hack was the last straw that broke
the camel's back, and that we are better off with a clean solution
straight away. If you send the clean approach i can help test it on a
good number of boxes.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

htejun at gmail

Nov 10, 2009, 11:44 AM

Post #7 of 29 (602 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Tejun Heo wrote:
> The return value is wrong but it wouldn't lead to oops.

Correction: It may lead to oops due to wrong end-of-list condition and
I just remembered that I was thinking about it when I was modifying
the locking and adding the switch block there and then I forgot to
actually update the return value of the extend function. So, yeah,
three way return value sucks big time.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 10, 2009, 11:50 AM

Post #8 of 29 (601 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Ingo Molnar wrote:
> * Tejun Heo <tj [at] kernel> wrote:
>
>> In this case, as the second function needs to release to free the old
>> map, the extra unlock/lock pair is actually necessary. Splitting into
>> two functions is fine but I think it would be better to fix it first
>> and then split them in following patches so that it can be bisected if
>> I screw up while splitting, right?
>
> i think Linus's point was that this hack was the last straw that broke
> the camel's back, and that we are better off with a clean solution
> straight away. If you send the clean approach i can help test it on a
> good number of boxes.

If you're talking about the locking itself, there really is no clean
solution at this point. Until vmalloc locking is updated, we'll have
to do locking impedance matching in percpu code. I'm still not quite
sure whether we really need to update vmalloc code to be irqsafe.

If you're talking about the three way return value, which I do agree
to be quite ugly, I think it will be a lot safer to have three patches
- one to fix the deadlock, another to fix the return value and the
final one to de-uglify the function, especially as we're pretty late
in the release cycle.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Nov 10, 2009, 1:42 PM

Post #9 of 29 (602 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

On Wed, 11 Nov 2009, Tejun Heo wrote:
>
> If you're talking about the three way return value, which I do agree
> to be quite ugly, I think it will be a lot safer to have three patches
> - one to fix the deadlock, another to fix the return value and the
> final one to de-uglify the function, especially as we're pretty late
> in the release cycle.

I'm certainly ok with doing it in stages if that is how you want to do it.

That said, I'm not entirely sure it's _worthwhile_, since the "return 1"
case has apparently never ever actually worked. From a bisect standpoint,
what's the difference between seeing

- oh, now that we made it return the documented code and actually re-try
properly when dropping the lock, it turns out that the re-try code was
always buggy and we just hadn't noticed before because it didn't
trigger

or

- oh, now that we rewrote the function to be cleaner and do the lock
dropping and retry more obviously, it turns out that the retry doesn't
actually work and leads to deadlocks.

but I don't care deeply.

I want the cleanup because I think that the code sucks from a "future
proofing" and readability standpoint, but I really don't mind one way or
the other whether you want to finally do that one "return 1" correctly for
one commit, only to then fix it to not do that three-way test of a single
function in the next one.

So whatever works - as long as the end result both looks sane and doesn't
have the bug we clearly have now.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 10, 2009, 7:55 PM

Post #10 of 29 (598 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello,

Linus Torvalds wrote:
> I want the cleanup because I think that the code sucks from a "future
> proofing" and readability standpoint, but I really don't mind one way or
> the other whether you want to finally do that one "return 1" correctly for
> one commit, only to then fix it to not do that three-way test of a single
> function in the next one.
>
> So whatever works - as long as the end result both looks sane and doesn't
> have the bug we clearly have now.

I was mostly worrying about introducing unrelated bug while changing.
Anyways, one patch it is. I'll route it through tip as suggested by
Ingo in a few hours.

Thanks a lot.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 11, 2009, 3:31 AM

Post #11 of 29 (596 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> Hello,
>
> Linus Torvalds wrote:
> > I want the cleanup because I think that the code sucks from a "future
> > proofing" and readability standpoint, but I really don't mind one way or
> > the other whether you want to finally do that one "return 1" correctly for
> > one commit, only to then fix it to not do that three-way test of a single
> > function in the next one.
> >
> > So whatever works - as long as the end result both looks sane and doesn't
> > have the bug we clearly have now.
>
> I was mostly worrying about introducing unrelated bug while changing.
> Anyways, one patch it is. I'll route it through tip as suggested by
> Ingo in a few hours.

Btw., i'd suggest you keep it in your percpu tree as usual and send it
to Linus directly - i offered testing for the cleanup patch (and can
pull your patch for such a purpose), it doesnt 'have' to go via -tip.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 11, 2009, 4:21 AM

Post #12 of 29 (596 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello, Ingo.

Ingo Molnar wrote:
>> I was mostly worrying about introducing unrelated bug while changing.
>> Anyways, one patch it is. I'll route it through tip as suggested by
>> Ingo in a few hours.
>
> Btw., i'd suggest you keep it in your percpu tree as usual and send it
> to Linus directly - i offered testing for the cleanup patch (and can
> pull your patch for such a purpose), it doesnt 'have' to go via -tip.

Can you please then pull from the following tree for testing? I'll
push it to Linus after a couple of days if nothing explodes.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git for-linus

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 11, 2009, 11:57 AM

Post #13 of 29 (586 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> Hello, Ingo.
>
> Ingo Molnar wrote:
> >> I was mostly worrying about introducing unrelated bug while changing.
> >> Anyways, one patch it is. I'll route it through tip as suggested by
> >> Ingo in a few hours.
> >
> > Btw., i'd suggest you keep it in your percpu tree as usual and send it
> > to Linus directly - i offered testing for the cleanup patch (and can
> > pull your patch for such a purpose), it doesnt 'have' to go via -tip.
>
> Can you please then pull from the following tree for testing? I'll
> push it to Linus after a couple of days if nothing explodes.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git for-linus
>
> Thanks.

Sure - pulled it into tip:master for testing earlier today and after a
few hours of it's looking good so far in x86 runtime tests. I also did
cross-build testing to a dozen non-x86 architectures and it was fine
there too.

btw., there's some 80-cols checkpatch warning artifacts in the commit:

+ if (pcpu_extend_area_map(chunk, new_alloc) < 0) {
+ err = "failed to extend area map of "
+ "reserved chunk";

which suggest that the logic here is perhaps nested a bit too deep. It
could be improved by moving the reserved allocation branch of
pcpu_alloc():

if (reserved && pcpu_reserved_chunk) {

into a helper inline function, something like __pcpu_alloc_reserved().

It's a rare special case anyway. It could be changed to return with the
pcpu_lock always taken, so the above branch would look like this:

if (unlikely(reserved)) {
off = __pcpu_alloc_reserved(&chunk, size, align, &err);
if (off < 0)
goto fail_unlock;
goto area_found;
}

Which is a cleaner flow IMO, and which simplifes pcpu_alloc().

And the error string should be:

err = "failed to extend area map of reserved chunk";

(Ignore the checkpatch complaint.)

What do you think?

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 12, 2009, 2:11 AM

Post #14 of 29 (579 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello, Ingo.

11/12/2009 04:57 AM, Ingo Molnar wrote:
> Sure - pulled it into tip:master for testing earlier today and after a
> few hours of it's looking good so far in x86 runtime tests. I also did
> cross-build testing to a dozen non-x86 architectures and it was fine
> there too.

Great.

> btw., there's some 80-cols checkpatch warning artifacts in the commit:
>
> + if (pcpu_extend_area_map(chunk, new_alloc) < 0) {
> + err = "failed to extend area map of "
> + "reserved chunk";
>
> which suggest that the logic here is perhaps nested a bit too deep. It
> could be improved by moving the reserved allocation branch of
> pcpu_alloc():

Strange, although the line break isn't the prettiest thing, the only
checkpatch problem I can see is the following.

> scripts/checkpatch.pl 0001-percpu-restructure-pcpu_extend_area_map-to-fix-bugs-.patch
ERROR: trailing whitespace
#80: FILE: mm/percpu.c:382:
+^Ireturn new_alloc;^I$

total: 1 errors, 0 warnings, 179 lines checked

0001-percpu-restructure-pcpu_extend_area_map-to-fix-bugs-.patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

The patch adds a trailing tab. I'll fix that up (I usually catch
these while using quilt but this one didn't go through quilt and I
forgot to run checkpatch).

> if (reserved && pcpu_reserved_chunk) {
>
> into a helper inline function, something like __pcpu_alloc_reserved().
>
> It's a rare special case anyway. It could be changed to return with the
> pcpu_lock always taken, so the above branch would look like this:
>
> if (unlikely(reserved)) {
> off = __pcpu_alloc_reserved(&chunk, size, align, &err);
> if (off < 0)
> goto fail_unlock;
> goto area_found;
> }
>
> Which is a cleaner flow IMO, and which simplifes pcpu_alloc().

Hmmm... The thing is that the nesting isn't that deep there and
breaking string in the middle is something we do quite often. What
checkpatch warning did you see?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 12, 2009, 2:36 AM

Post #15 of 29 (577 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> Hmmm... The thing is that the nesting isn't that deep there and
> breaking string in the middle is something we do quite often. What
> checkpatch warning did you see?

( i did not run checkpatch over your commit - i just assumed that the
ugliness was a checkpatch artifact. )

Breaking strings mid-sentence is something we try not to do. (If you
know about places that do it 'quite often' then those places need fixing
too.)

The biggest problem with it s that it breaks git grep. If someone sees
this message in the log:

PERCPU: allocation failed, size=1024 align=32, failed to extend area map of reserved chunk

and types the obvious:

git grep 'failed to extend area map of reserved chunk'

the git-grep comes up empty because the string was needlessly broken in
mid-sentence. Which is a confusing result and which causes people to
waste time trying to figure out where the message came from.

The other messages in this function are fine btw, for example:

git grep 'failed to populate'

will come up with the right place.

( There are also other reasons why we dont break strings mid-sentence -
it's also less readable to have it on two lines. )

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 12, 2009, 2:58 AM

Post #16 of 29 (577 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello, Ingo.

11/12/2009 07:36 PM, Ingo Molnar wrote:
>
> * Tejun Heo <tj [at] kernel> wrote:
>
>> Hmmm... The thing is that the nesting isn't that deep there and
>> breaking string in the middle is something we do quite often. What
>> checkpatch warning did you see?
>
> ( i did not run checkpatch over your commit - i just assumed that the
> ugliness was a checkpatch artifact. )
>
> Breaking strings mid-sentence is something we try not to do. (If you
> know about places that do it 'quite often' then those places need fixing
> too.)

Oh... I do that all the time and I see a lot of them around too.

> the git-grep comes up empty because the string was needlessly broken in
> mid-sentence. Which is a confusing result and which causes people to
> waste time trying to figure out where the message came from.
>
> The other messages in this function are fine btw, for example:
>
> git grep 'failed to populate'
>
> will come up with the right place.

While I agree this is a valid reason, I really don't think we should
be restructuring whole code to accomodate long strings on single line.
I think a better way would be to teach grepping tool to match those
broken lines. It shouldn't be too difficult to put this into ack[1]
and maybe we can have git-ack (that's a bad name for git tho). I'll
ask ack author nicely.

> ( There are also other reasons why we dont break strings mid-sentence -
> it's also less readable to have it on two lines. )

This really depends on personal tastes. When trying to use long
string literals, there are several choices.

1. Use broken strings.

printk("blah blah blah blah "
"blah blah blah blah\n");

2. Push it into new line and unindent it.

printk(
"blah blah blah blah blah blah blah blah\n");

3. Restructure code so that the literal ends up in outer block.

printk("blah blah blah blah blah blah blah blah\n");

I prefer the first choice. The third would be nice if it's trivial to
do but I don't think it should dictate the code structure. The second
one, I don't know. Some people like that and grep will be happy with
it but it just seems very disturbing to my eyes.

Thanks.

--
tejun

[1] http://betterthangrep.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 12, 2009, 3:07 AM

Post #17 of 29 (578 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> > if (reserved && pcpu_reserved_chunk) {
> >
> > into a helper inline function, something like __pcpu_alloc_reserved().
> >
> > It's a rare special case anyway. It could be changed to return with the
> > pcpu_lock always taken, so the above branch would look like this:
> >
> > if (unlikely(reserved)) {
> > off = __pcpu_alloc_reserved(&chunk, size, align, &err);
> > if (off < 0)
> > goto fail_unlock;
> > goto area_found;
> > }
> >
> > Which is a cleaner flow IMO, and which simplifes pcpu_alloc().
>
> Hmmm... The thing is that the nesting isn't that deep there [...]

Well, the pcpu_alloc() function is 115 lines which is a bit long. It
does 2-3 things while a function should try to do one thing.

Putting the reserved allocation into a separate function also makes the
'main' path of logic more visible and obstructed less by rare details.

The indentation i pinpointed is 4 levels deep:

err = "failed to extend area map of "
"reserved chunk";

which is a bit too much IMO - the code starts in the middle of the
screen, there's barely any space to do anything meaningful.

But there's other line wrap artifacts as well further down:

if (pcpu_extend_area_map(chunk,
new_alloc) < 0) {

But ... there's no hard rules here and i've seen functions where 4
levels of indentation were just ok. Anyway, i just gave you my opinion,
and i'm definitely more on the nitpicky side of the code quality
equilibrium, YMMV.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

mingo at elte

Nov 12, 2009, 3:25 AM

Post #18 of 29 (577 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

* Tejun Heo <tj [at] kernel> wrote:

> This really depends on personal tastes. When trying to use long
> string literals, there are several choices.
>
> 1. Use broken strings.
>
> printk("blah blah blah blah "
> "blah blah blah blah\n");
>
> 2. Push it into new line and unindent it.
>
> printk(
> "blah blah blah blah blah blah blah blah\n");
>
> 3. Restructure code so that the literal ends up in outer block.
>
> printk("blah blah blah blah blah blah blah blah\n");
>
> I prefer the first choice. The third would be nice if it's trivial to
> do but I don't think it should dictate the code structure. The second
> one, I don't know. Some people like that and grep will be happy with
> it but it just seems very disturbing to my eyes.

My preferred choice is:

4. Change "blah blah blah blah blah blah blah blah\n" to "blah\n" -
the user really does not win much from the repetition.

Seriously, if you _ever_ get into a 'hm, the string is too long'
situation, you should ask yourself two fundamental questions:

a) Is the user really interested in this small novel?

b) Is this a side-effect of a bloated function having too deep
indentation?

IMHO, in the specific case at hand, both a) and b) apply:

err = "failed to extend area map of "
"reserved chunk";

the indentation is a bit too deep, and the message is too chatty - the
output itself is:

PERCPU: allocation failed, size=1024 align=32, failed to extend area map of reserved chunk

A better/shorter message would be:

percpu: 1024 bytes allocation failed: could not extend reserved chunk area map

This formulation is a bit shorter and i doubt the align parameter really
needs printing - it's almost always the same and other context will tell
us what it is anyway.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 12, 2009, 3:29 AM

Post #19 of 29 (577 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello, Ingo.

11/12/2009 08:07 PM, Ingo Molnar wrote:
> Well, the pcpu_alloc() function is 115 lines which is a bit long. It
> does 2-3 things while a function should try to do one thing.

I agree for low level / utility functions but for top level functions
which direct the flow of the whole logic, I usually prefer to put them
flat. To me, that way things seem less obfuscated.

> Putting the reserved allocation into a separate function also makes the
> 'main' path of logic more visible and obstructed less by rare details.
>
> The indentation i pinpointed is 4 levels deep:
>
> err = "failed to extend area map of "
> "reserved chunk";
>
> which is a bit too much IMO - the code starts in the middle of the
> screen, there's barely any space to do anything meaningful.

Well, all that's there is error exit. Surrounding code segment is,

if (pcpu_extend_area_map(chunk, new_alloc) < 0) {
err = "failed to extend area map of "
"reserved chunk";
goto fail_unlock_mutex;
}

So, we might as well just do

err = "failed to extend area map of reserved chunk";
if (pcpu_extend_area_map(chunk, new_alloc) < 0)
goto fail_unlock_mutex;

> But there's other line wrap artifacts as well further down:
>
> if (pcpu_extend_area_map(chunk,
> new_alloc) < 0) {

This one is uglier and one level deeper than the previous one. The
resulting nesting was one of the reasons why I factored out
pcpu_extend_area_map() as a whole and switched on the return value but
that obfuscated locking. Although it nests quite a bit, I don't think
the loop there is too bad. It shows what it does pretty well.

> But ... there's no hard rules here and i've seen functions where 4
> levels of indentation were just ok. Anyway, i just gave you my opinion,
> and i'm definitely more on the nitpicky side of the code quality
> equilibrium, YMMV.

Indentation and code style are actually something I end up spending
quite some time on and I did think about the second one. Factoring
out without hiding locking is a bit difficult but if I rename
new_alloc to new_len, I can fit that thing onto a single line but that
would probably require renaming matching local variable in
pcpu_extend_area_map() which will end up generating unnecessary amount
of diff obfuscating the real change. At that point, I just thought we
could live with one slightly ugly line break.

So, I don't know. Pros and cons on these things depend too much on
personal tastes (and even mood at the time of writing) to form uniform
standard to follow.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

oliver at neukum

Nov 12, 2009, 6:26 AM

Post #20 of 29 (573 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Am Donnerstag, 12. November 2009 11:58:19 schrieb Tejun Heo:

> While I agree this is a valid reason, I really don't think we should
> be restructuring whole code to accomodate long strings on single line.
> I think a better way would be to teach grepping tool to match those
> broken lines. It shouldn't be too difficult to put this into ack[1]
> and maybe we can have git-ack (that's a bad name for git tho). I'll
> ask ack author nicely.

There's a point where following style guidelines turns into a fetish.
Plain simple "grep" should work.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Nov 12, 2009, 7:17 AM

Post #21 of 29 (574 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

On Thu, 12 Nov 2009, Tejun Heo wrote:
>
> 11/12/2009 07:36 PM, Ingo Molnar wrote:
> >
> > Breaking strings mid-sentence is something we try not to do. (If you
> > know about places that do it 'quite often' then those places need fixing
> > too.)
>
> Oh... I do that all the time and I see a lot of them around too.

I hate them. I do greps for error messages, and it's annoying as hell if
it's hard to find.

'checkpatch' is the major reason for them, but I think we've fixed
checkpath long ago to not warn about long lines if they are due to a long
string.

Strings should basically be broken up only at '\n' characters, so

printk("This is a made-up example.\n"
"Ok like this\n");

is fine, because you can expect to grep for "made-up example", but not
over the newline.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tj at kernel

Nov 12, 2009, 7:30 AM

Post #22 of 29 (573 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

Hello,

11/13/2009 12:17 AM, Linus Torvalds wrote:
> On Thu, 12 Nov 2009, Tejun Heo wrote:
>> 11/12/2009 07:36 PM, Ingo Molnar wrote:
>>> Breaking strings mid-sentence is something we try not to do. (If you
>>> know about places that do it 'quite often' then those places need fixing
>>> too.)
>>
>> Oh... I do that all the time and I see a lot of them around too.
>
> I hate them. I do greps for error messages, and it's annoying as hell if
> it's hard to find.
>
> 'checkpatch' is the major reason for them, but I think we've fixed
> checkpath long ago to not warn about long lines if they are due to a long
> string.
>
> Strings should basically be broken up only at '\n' characters, so
>
> printk("This is a made-up example.\n"
> "Ok like this\n");
>
> is fine, because you can expect to grep for "made-up example", but not
> over the newline.

If the consensus is to allow long string literals to overrun 80 column
limit, I have no objection. It makes code slightly more difficult to
read for some people but well we can't make everyone happy.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

htejun at gmail

Nov 12, 2009, 7:45 AM

Post #23 of 29 (573 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

11/13/2009 12:30 AM, Tejun Heo wrote:
>> 'checkpatch' is the major reason for them, but I think we've fixed
>> checkpath long ago to not warn about long lines if they are due to a long
>> string.

It still warns... :-(

WARNING: line over 80 characters
#11: FILE: mm/percpu.c:1100:
+ err = "failed to extend area map of reserved chunk";

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Nov 12, 2009, 7:52 AM

Post #24 of 29 (573 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

On Fri, 13 Nov 2009, Tejun Heo wrote:

> 11/13/2009 12:30 AM, Tejun Heo wrote:
> >> 'checkpatch' is the major reason for them, but I think we've fixed
> >> checkpath long ago to not warn about long lines if they are due to a long
> >> string.
>
> It still warns... :-(
>
> WARNING: line over 80 characters
> #11: FILE: mm/percpu.c:1100:
> + err = "failed to extend area map of reserved chunk";

Ok, just ignore it. The 80-column warning has always been sh*t anyway.

It may be that the "long line" warning is suppressed only for actual
'printk()' cases, but it may also be that it looks at the intendation and
only suppresses it for low levels of indents. I'm allergic to perl, so I'm
not even going to look to try to find out the details.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

andresbaldrich at ymail

Nov 12, 2009, 9:04 AM

Post #25 of 29 (573 views)
Permalink

Re: [GIT PULL] percpu fixes for 2.6.32-rc6 [In reply to]

El jue 12-nov-09, Linus Torvalds <torvalds [at] linux-foundation> escribi�:
> On Thu, 12 Nov 2009, Tejun Heo wrote:
> >
> > 11/12/2009 07:36 PM, Ingo Molnar wrote:
> > >
> > > Breaking strings mid-sentence is something we try
> not to do. (If you
> > > know about places that do it 'quite often' then
> those places need fixing
> > > too.)
> >
> > Oh... I do that all the time and I see a lot of them
> around too.
>
> I hate them. I do greps for error messages, and it's
> annoying as hell if
> it's hard to find.
>
> 'checkpatch' is the major reason for them, but I think
> we've fixed
> checkpath long ago to not warn about long lines if they are
> due to a long
> string.
>
> Strings should basically be broken up only at '\n'
> characters, so
>
> �� printk("This is a made-up example.\n"
> �� "Ok like this\n");
>
> is fine, because you can expect to grep for "made-up
> example", but not
> over the newline.
>
> ��
> �� Linus

(Kernel)/Documentation/CodingStyle
line 83:
"Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are placed
substantially to the right. The same applies to function headers with a long
argument list. Long strings are as well broken into shorter strings. The
only exception to this is where exceeding 80 columns significantly increases
readability and does not hide information.

void fun(int a, int b, int c)
{
if (condition)
printk(KERN_WARNING "Warning this is a long printk with "
"3 parameters a: %u b: %u "
"c: %u \n", a, b, c);
else
next_statement;
}"

Perhaps it should be changed?
I read the list's faq, it asked useful code or "diff" outputs for proposed changes, but i am an *absolute* beginner with < 6 months of college. 'Never' used FOSS before.
I hope it was good imput.
Andr�s Baldrich <abaldric [at] alu>

Yahoo! Cocina

Encontra las mejores recetas con Yahoo! Cocina.

http://ar.mujer.yahoo.com/cocina/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

1 2

View All

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads