Re: [PATCH] x86: mtrr cleanup for converting continuous to discrete layout v5

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 28 Apr 2008 15:05:05 -0700 Yinghai Lu <yhlu.kernel.send@gmail.com> wrote:

> 
> some BIOS like to use continus MTRR layout, and may X driver can not add
> WB entries for graphical cards when 4g or more RAM installed.
> 
> the patch will change MTRR to discrete.
> 
> mtrr_chunk_size= could be used to have smaller continuous block to hold holes.
> default is 256m, could be set according to size of graphics card memory.
> 
> v2: fix -1 for UC checking
> v3: default to disable, and need use enable_mtrr_cleanup to enable this feature
>     skip the var state change warning.
>     remove next_basek in range_to_mtrr()
> v4: correct warning mask.
> v5: CONFIG_MTRR_SANITIZER
> 
> Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

> +#ifdef CONFIG_MTRR_SANITIZER
> +
> +#ifdef CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT

I don't think these newly-added config items should exist, sorry.  But
then, the changelog does't describe _why_ they exist (it should!) and I
probably missed it in the discusson.

Anyone who distributes a kernel will need to enable both
CONFIG_MTRR_SANITIZER and CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT, so the
config items are only useful for saving a bit of kernel text in custom
kernel builds.

> +static int enable_mtrr_cleanup __initdata = 1;
> +#else
> +static int enable_mtrr_cleanup __initdata;

The disable_mtrr_cleanup and enable_mtrr_cleanup boot options are also
problematic.  We really really want this stuff to all happen automatically.

What happens with this sort of thing is that people's machines misbehave
and I expect most of them never find out about the magic option.  They
give up on Linux or use a different computer or use a different distro
which happened to set the option the other way, etc, etc.  Some people will
think to do a bit of googling and might stumble across the option after a
while.

It's all rather user-unfriendly and we should try really hard to just make
things work.  Is this at all possible?


Anyway.  I think the problem which you have identified is solveable in
userspace, isn't it?  Read the existing mtrr settings and rewrite them in a
better form?  If so, we could prepare a little program which does that and
make the X people and distributors aware of it.  This has the significant
advantage that it will fix pre-2.6.26 kernels too.

> +#endif
> +
> +#else
> +
> +static int enable_mtrr_cleanup __initdata = -1;
> +
> +#endif
> +
> +static int __init disable_mtrr_cleanup_setup(char *str)
> +{
> +	if (enable_mtrr_cleanup != -1)
> +		enable_mtrr_cleanup = 0;
> +	return 0;
> +}
> +early_param("disable_mtrr_cleanup", disable_mtrr_cleanup_setup);
> +
> +static int __init enable_mtrr_cleanup_setup(char *str)
> +{
> +	if (enable_mtrr_cleanup != -1)
> +		enable_mtrr_cleanup = 1;
> +	return 0;
> +}
> +early_param("enble_mtrr_cleanup", enable_mtrr_cleanup_setup);
> +
> +#define RANGE_NUM 256
> +
> +struct res_range {
> +	size_t start;
> +	size_t end;
> +};

size_t is a surprising choice of type.

> +static void __init subtract_range(struct res_range *range, size_t start,
> +				size_t end)
> +{
> +	int i;
> +	int j;
> +
> +	for (j = 0; j < RANGE_NUM; j++) {
> +		if (!range[j].end)
> +			continue;
> +
> +		if (start <= range[j].start && end >= range[j].end) {
> +			range[j].start = 0;
> +			range[j].end = 0;
> +			continue;
> +		}
> +
> +		if (start <= range[j].start && end < range[j].end && range[j].start < end + 1) {

We prefer that code remain within 0 columns, please.

> +			range[j].start = end + 1;
> +			continue;
> +		}
> +
> +
> +		if (start > range[j].start && end >= range[j].end && range[j].end > start - 1) {
> +			range[j].end = start - 1;
> +			continue;
> +		}
> +
> +		if (start > range[j].start && end < range[j].end) {
> +			/* find the new spare */
> +			for (i = 0; i < RANGE_NUM; i++) {
> +				if (range[i].end == 0)
> +					break;
> +			}
> +			if (i < RANGE_NUM) {
> +				range[i].end = range[j].end;
> +				range[i].start = end + 1;
> +			} else {
> +				printk(KERN_ERR "run of slot in ranges\n");
> +			}
> +			range[j].end = start - 1;
> +			continue;
> +		}
> +	}
> +}
> +
> +static int __cpuinit cmp_range(const void *x1, const void *x2)

You wanted __init here.