All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Tejun Heo <tj@kernel.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Fr??d??ric Weisbecker <fweisbec@gmail.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Christoph Lameter <cl@linux-foundation.org>,
	linux-next@vger.kernel.org, linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: linux-next: percpu tree build warning
Date: Sun, 29 Nov 2009 07:40:19 +0100	[thread overview]
Message-ID: <20091129064019.GA19916@elte.hu> (raw)
In-Reply-To: <200911282021.50315.rusty@rustcorp.com.au>


* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Fri, 27 Nov 2009 04:11:28 pm Ingo Molnar wrote:
> > 
> > * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > 
> > > On Thu, 26 Nov 2009 12:10:58 am Ingo Molnar wrote:
> > > > While a percpu variable is defined and used in completely different 
> > > > ways:
> > > > 
> > > >   DEFINE_PER_CPU(unsigned long, dr7);
> > > > 
> > > > and is used via:
> > > > 
> > > >   __get_cpu_var(dr7);  [[Fixed -- RR]]
> > > 
> > > The entire point of Tejun's per-cpu work is that &dr7 is now valid. A 
> > > per-cpu pointer as if it were allocated by the dynamic per-cpu 
> > > allocator.
> > >
> > > Your arguments are fine, but out-of-date.
> > 
> > But allowing &dr7 is outright dangerous - and not particularly clean 
> > either.
> 
> That's foolish.  We can now have generic per-cpu function for counters 
> and the like.

So your argument in favor of what i see as at least a mild form of type 
obfusaction is that ... even more obfuscation is upcoming?

I think percpu usage should be spelled out clear and loud. We should not 
pretend they are 'usual' C variables, because they are not. They are 
defined in a special way, they are used via special operators. I sure 
want to make sure that taking an address of one of them:

	ptr = &dr7;

... looks special too.

Just look at the two 'fixes' i quoted in this discussion:

 28b4e0d: x86: Rename global percpu symbol dr7 to cpu_dr7
 11e6635: kernel/hw_breakpoint.c: Fix local/global shadowing

They actually 'solved' the shadowing by renaming the variables to ... 
cpu_. Think about it: the 'I am percpu' prefix came right back - it's 
just now present in a more volatile form and the default usage is 
slightly more dangerous!

I guess i'm a bit more sensitive to percpu complications than you 
because i've seen my fair share of bugs in the scheduler (and 
preemptible/non-preemptible code) related to percpu code (a fair share 
of it introduced by yourself ;-), so the last thing i'd like to see is 
changes that are hiding its nature.

I _use_ percpu code, i dont just write the facilities ;-)

> [...] Again, I'm explaining what you should already know before 
> sending email about this stuff.
> [...]
> Stupidest debate ever.

What i am making is a somewhat subtle technical argument and making any 
progress on it needs at least a minimal form of a working debate. I do 
not claim i am right, but still you are dismissing my arguments in a 
rather nasty way.

... alas, i dont care _that_ much about this and i dont think my 
concerns deserved your ad hominem attacks so i see no point in further 
participating in this thread.

Thanks,

	Ingo

  reply	other threads:[~2009-11-29  6:40 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-25 10:42 linux-next: percpu tree build warning Stephen Rothwell
2009-11-25 10:50 ` Ingo Molnar
2009-11-25 11:14   ` Rusty Russell
2009-11-25 11:58     ` Ingo Molnar
2009-11-25 12:39       ` Tejun Heo
2009-11-25 13:42       ` [tip:perf/core] x86: Rename global percpu symbol dr7 to cpu_dr7 tip-bot for Tejun Heo
2009-11-25 12:31   ` linux-next: percpu tree build warning Tejun Heo
2009-11-25 13:40     ` Ingo Molnar
2009-11-25 15:12       ` Tejun Heo
2009-11-26 22:16       ` Rusty Russell
2009-11-27  5:41         ` Ingo Molnar
2009-11-27  5:57           ` Tejun Heo
2009-11-27  6:20             ` Ingo Molnar
2009-11-27  6:31               ` Tejun Heo
2009-11-27  6:32                 ` Tejun Heo
2009-11-28  9:51           ` Rusty Russell
2009-11-29  6:40             ` Ingo Molnar [this message]
2009-11-30  0:31               ` Rusty Russell
2009-11-25 13:24   ` [PATCH] x86: rename global percpu symbol dr7 to cpu_dr7 Tejun Heo
  -- strict thread matches above, loose matches on Subject: below --
2009-11-12  6:45 linux-next: percpu tree build warning Stephen Rothwell
2009-11-12 15:16 ` Christoph Lameter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091129064019.GA19916@elte.hu \
    --to=mingo@elte.hu \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux-foundation.org \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=sfr@canb.auug.org.au \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.