linux-kernel - Re: [PATCH 17/24] OProfile: Enable IBS for AMD CPUs

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: <20080726100937.GI25890@elte.hu>
Date:	Sat, 26 Jul 2008 12:09:37 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Robert Richter <robert.richter@....com>
Cc:	Barry Kasindorf <barry.kasindorf@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	oprofile-list <oprofile-list@...ts.sourceforge.net>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 17/24] OProfile: Enable IBS for AMD CPUs


* Robert Richter <robert.richter@....com> wrote:

> +	/* default values, can be overwritten by model */
> +	ops->create_files = nmi_create_files;
> +	ops->setup = nmi_setup;
> +	ops->shutdown = nmi_shutdown;
> +	ops->start = nmi_start;
> +	ops->stop = nmi_stop;
> +	ops->cpu_type = cpu_type;

i know you are moving existing code around, but sill it helps 
readability if you align these vertically too, like:

> +	/* default values, can be overwritten by model */
> +	ops->create_files	= nmi_create_files;
> +	ops->setup		= nmi_setup;
> +	ops->shutdown		= nmi_shutdown;
> +	ops->start		= nmi_start;
> +	ops->stop		= nmi_stop;
> +	ops->cpu_type		= cpu_type;

if i look at the aligned variant during review, i can see it immediately 
that it's fine and that left and right matches up conceptually. I can 
also see it, without having to look anywhere else, that ->cpu_type is 
special.

> @@ -482,11 +494,15 @@ static int setup_ibs_files(struct super_block * sb, struct dentry * root)
>  
>  static int op_amd_init(struct oprofile_operations *ops)
>  {
> +	setup_ibs();
> +	create_arch_files = ops->create_files;
> +	ops->create_files = setup_ibs_files;
>  	return 0;

the (non-)locking might be a bit racy here: the oprofilefs entries are 
set up first in setup_ibs() and made visible before you override them. 
oprofilefs entries should be made visible at the very last step, when 
all pointers are at their final versions and are stable already.

	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/