Re: [PATCH 11/24] x86/oprofile: Add IBS support for AMD CPUs, model specific code

[Ingo Molnar <mingo@elte.hu>]

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

> @@ -132,6 +214,65 @@ static int op_amd_check_ctrs(struct pt_regs * const regs,
>  		}
>  	}
>  
> +	/*If AMD and IBS is available */
> +	if (ibs_allowed && ibs_config.fetch_enabled) {
> +		rdmsr(MSR_AMD64_IBSFETCHCTL, low, high);
> +		if (high & IBS_FETCH_VALID_BIT) {
> +			ibs_fetch.ibs_fetch_ctl_high = high;
> +			ibs_fetch.ibs_fetch_ctl_low = low;
> +			rdmsr(MSR_AMD64_IBSFETCHLINAD, low, high);
> +			ibs_fetch.ibs_fetch_lin_addr_high = high;
> +			ibs_fetch.ibs_fetch_lin_addr_low = low;
> +			rdmsr(MSR_AMD64_IBSFETCHPHYSAD, low, high);
> +			ibs_fetch.ibs_fetch_phys_addr_high = high;
> +			ibs_fetch.ibs_fetch_phys_addr_low = low;
> +
> +			oprofile_add_ibs_sample(regs,
> +						(unsigned int *)&ibs_fetch,
> +						IBS_FETCH_BEGIN);

please move this innermost code into a helper function. That will help 
eliminate line 80 artifacts as well.

> +	if (ibs_allowed && ibs_config.op_enabled) {
> +		rdmsr(MSR_AMD64_IBSOPCTL, low, high);
> +		if (low & IBS_OP_VALID_BIT) {
> +			rdmsr(MSR_AMD64_IBSOPRIP, low, high);
> +			ibs_op.ibs_op_rip_low = low;
> +			ibs_op.ibs_op_rip_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA, low, high);
> +			ibs_op.ibs_op_data1_low = low;
> +			ibs_op.ibs_op_data1_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA2, low, high);
> +			ibs_op.ibs_op_data2_low = low;
> +			ibs_op.ibs_op_data2_high = high;
> +			rdmsr(MSR_AMD64_IBSOPDATA3, low, high);
> +			ibs_op.ibs_op_data3_low = low;
> +			ibs_op.ibs_op_data3_high = high;
> +			rdmsr(MSR_AMD64_IBSDCLINAD, low, high);
> +			ibs_op.ibs_dc_linear_low = low;
> +			ibs_op.ibs_dc_linear_high = high;
> +			rdmsr(MSR_AMD64_IBSDCPHYSAD, low, high);
> +			ibs_op.ibs_dc_phys_low = low;
> +			ibs_op.ibs_dc_phys_high = high;
> +
> +			/* reenable the IRQ */
> +			oprofile_add_ibs_sample(regs,
> +						(unsigned int *)&ibs_op,
> +						IBS_OP_BEGIN);
> +			rdmsr(MSR_AMD64_IBSOPCTL, low, high);
> +			low &= ~(IBS_OP_VALID_BIT);
> +			low |= IBS_OP_ENABLE;
> +			wrmsr(MSR_AMD64_IBSOPCTL, low, high);
> +		}
> +	}

ditto.

> +		/**** Be sure to run loop until NULL is returned to
> +		decrement reference count on any pci_dev structures
> +		returned ****/

please use standard comment style:

 /*
  * Multi-line ..........
  * ....................... comment:
  */

> +		while ((gh_device = pci_get_device(PCI_VENDOR_ID_AMD,
> +			PCI_DEVICE_ID_AMD_10H_NB_MISC, gh_device))
> +			!= NULL) {
> +			/* This code may change if we can find a proper
> +			* way to get at the PCI extended config space */

ditto.

> +			pci_write_config_dword(
> +				gh_device, IBS_LVT_OFFSET_PCI,
> +				(vector | IBS_CTL_LVT_OFFSET_VALID_BIT));

perhaps use a helper function to reduce col 80 artifacts.

> +/*
> + * unitialize the APIC for the IBS interrupts if needed on AMD Family10h
> + * rev B0 and later */

comment style.

> +static void setup_ibs_files(struct super_block *sb, struct dentry *root)
> +{
> +	char buf[12];

magic limit.

> +	struct dentry *dir;
> +
> +	if (!ibs_allowed)
> +		return;
> +
> +	/* setup some reasonable defaults */
> +	ibs_config.max_cnt_fetch = 250000;
> +	ibs_config.fetch_enabled = 0;
> +	ibs_config.max_cnt_op = 250000;
> +	ibs_config.op_enabled = 0;
> +	ibs_config.dispatched_ops = 1;
> +	snprintf(buf,  sizeof(buf), "ibs_fetch");
> +	dir = oprofilefs_mkdir(sb, root, buf);

you could make use of a small style trick here: use a newline before 
this statement, to make blocks of code stand out in a clearer way. The 
construction of 'buf' is finished after the snprintf above, then comes 
the next step, to create the oprofilefs entries.

	Ingo