Messages in this thread |  | | | Date | Sat, 26 Jul 2008 12:09:37 +0200 | | From | Ingo Molnar <> | | Subject | Re: [PATCH 17/24] OProfile: Enable IBS for AMD CPUs |
| |
* Robert Richter <robert.richter@amd.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
|  |