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

From: Andrew Morton
Date: Mon Apr 28 2008 - 22:43:04 EST


On Mon, 28 Apr 2008 15:05:05 -0700 Yinghai Lu <yhlu.kernel.send@xxxxxxxxx> 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@xxxxxxxxx>

> +#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.


--
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/