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

[Ingo Molnar]

* Robert Richter <robert.richter@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/