linux-kernel - Re: linux-next: percpu tree build warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091125115856.GA17856@elte.hu>
Date:	Wed, 25 Nov 2009 12:58:56 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Stephen Rothwell <sfr@...b.auug.org.au>,
	Fr??d??ric Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Tejun Heo <tj@...nel.org>,
	Christoph Lameter <cl@...ux-foundation.org>,
	linux-next@...r.kernel.org, linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: linux-next: percpu tree build warning


(Cc:-ed Linus and Andrew)

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

> On Wed, 25 Nov 2009 09:20:04 pm Ingo Molnar wrote:
> > If yes then that needs to be fixed in the percpu tree. per-cpu variables 
> > used to have a __per_cpu prefix and that should be maintained - the two 
> > namespaces are obviously separate on the logical space, so they should 
> > never overlap in the implementational space either.
> 
> No, we've been through this.
> 
> sparse annotations replace the per_cpu prefix now per-cpu vars can be 
> used withn other than per-cpu ops (ie. their address can be usefully 
> taken).
> 
> The prefix crutch predated sparse.  And it was certainly never 
> supposed to let people write confusing and crap code like this.

What's confusing and crap about the code below? I dont think it is 
confusing, nor crap:

int arch_install_hw_breakpoint(struct perf_event *bp)
{
	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
	unsigned long *dr7;
	int i;

	for (i = 0; i < HBP_NUM; i++) {
		struct perf_event **slot = &__get_cpu_var(bp_per_reg[i]);

		if (!*slot) {
			*slot = bp;
			break;
		}
	}

	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
		return -EBUSY;

	set_debugreg(info->address, i);
	__get_cpu_var(cpu_debugreg[i]) = info->address;

	dr7 = &__get_cpu_var(dr7);
	*dr7 |= encode_dr7(i, info->len, info->type);

	set_debugreg(*dr7, 7);

	return 0;
}

This is basically equivalent to:

	pid = task->pid;

the 'dr7' in the local scope is clearly different from the 
__get_cpu_var(dr7) variable.

percpu variables are basically in a special struct. It's not like you 
can _ever_ access 'dr7' the percpu variable like that - it _always_ has 
to go via a proper percpu wrapper construct. So this change is 
needlessly obtrusive.

Really, guys, while the workaround is easy (a rename), this might be 
going a bit too far. I already think that the recently introduced 
limitation to name local percpu symbols globally sucked - but i'm not 
sure whether this new rule of not allowing such clear and clean looking 
code is acceptable.

Percpu variables now pollute _both_ the global and the local namespace - 
i dont think you can have it both ways.

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