Messages in this thread | | | Date | Tue, 29 Apr 2008 10:25:31 -0700 | From | "Yinghai Lu" <> | Subject | Re: [PATCH 1/2] x86: mtrr cleanup for converting continuous to discrete layout v8 |
| |
On Tue, Apr 29, 2008 at 6:07 AM, Ingo Molnar <mingo@elte.hu> wrote: > > a few minor cleanliness observations: > > > > +#ifdef CONFIG_MTRR_SANITIZER > > + > > +#ifdef CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT > > +static int enable_mtrr_cleanup __initdata = 1; > > +#else > > +static int enable_mtrr_cleanup __initdata; > > +#endif > > + > > +#else > > + > > +static int enable_mtrr_cleanup __initdata = -1; > > + > > +#endif > > this should be a single: > > #ifdef CONFIG_MTRR_SANITIZER > static int mtrr_cleanup_enabled = CONFIG_MTRR_SANITIZER_DEFAULT; > #endif > > block. > > > +#define RANGE_NUM 256 > > small explaination (comment) about what the limit means. > > > > +static int __init add_range(struct res_range *range, int nr_range, unsigned long start, > > + unsigned long end, int merge) > > looks cleaner this way: > > > static int __init > add_range(struct res_range *range, int nr_range, unsigned long start, > unsigned long end, int merge) > > > +{ > > + int i; > > + > > + if (!merge) > > + goto addit; > > + > > + /* try to merge it with old one */ > > + for (i = 0; i < nr_range; i++) { > > + unsigned long final_start, final_end; > > + unsigned long common_start, common_end; > > + > > + if (!range[i].end) > > + continue; > > + > > + common_start = max(range[i].start, start); > > + common_end = min(range[i].end, end); > > + if (common_start > common_end + 1) > > + continue; > > + > > + final_start = min(range[i].start, start); > > + final_end = max(range[i].end, end); > > + > > + range[i].start = final_start; > > + range[i].end = final_end; > > + return nr_range; > > + } > > + > > +addit: > > perhaps factor out the loop into a separate function and avoid the goto. > > > > +static void __init subtract_range(struct res_range *range, unsigned long start, > > + unsigned long end) > > should be: > > > static void __init > subtract_range(struct res_range *range, unsigned long start, > unsigned long end) > > > + int i; > > + int j; > > can be: > > int i, j; > > > + } > > + > > + > > stale newline. > > > > + if (start > range[j].start && end >= range[j].end && range[j].end > start - 1) { > > should be some sort of more readable in_range() check? > > > > + 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; > > + } > > + } > > +} > > > > +struct var_mtrr_state { > > + unsigned long range_startk, range_sizek; > > + unsigned long chunk_sizek; > > + unsigned long gran_sizek; > > + unsigned int reg; > > + unsigned address_bits; > > +}; > > s/unsigned address_bits/unsigned int address_bits/ > > also move range_sizek on a separate line. > > plus we tend to align structures this way: > > > +struct var_mtrr_state { > > + unsigned long range_startk; > > + unsigned long range_sizek; > > > + unsigned long chunk_sizek; > > + unsigned long gran_sizek; > > + unsigned int reg; > > + unsigned int address_bits; > > +}; > > (to put the types and field names into a visually more consistent form) > > > > +static void __init set_var_mtrr( > > + unsigned int reg, unsigned long basek, unsigned long sizek, > > + unsigned char type, unsigned address_bits) > > should be: > > > static void __init > set_var_mtrr(unsigned int reg, unsigned long basek, unsigned long sizek, > unsigned char type, unsigned address_bits) > > > + u32 base_lo, base_hi, mask_lo, mask_hi; > > + unsigned address_mask_high; > > s/unsigned/unsigned int > > hm, will this work on 64-bit? Above-4G is controlled via separate > mechanisms though so i guess it does. > > > > + address_mask_high = ((1u << (address_bits - 32u)) - 1u); > > use alignment macros instead. > > > > + unsigned long sizek; > > + /* Compute the maximum size I can make a range */ > > + if (range_startk) > > put extra newline between variable definition and code. > > > > + var_state.range_startk = 0; > > + var_state.range_sizek = 0; > > + var_state.reg = 0; > > + var_state.address_bits = address_bits; > > + var_state.chunk_sizek = mtrr_chunk_size >> 10; > > + var_state.gran_sizek = mtrr_gran_size >> 10; > > initialization looks nicer with vertical alignment, i.e.: > > > > + var_state.range_startk = 0; > > + var_state.range_sizek = 0; > > + var_state.reg = 0; > > + var_state.address_bits = address_bits; > > + var_state.chunk_sizek = mtrr_chunk_size >> 10; > > + var_state.gran_sizek = mtrr_gran_size >> 10; > > > > + /* Clear out the extra MTRR's */ > > + while (var_state.reg < num_var_ranges) > > + set_var_mtrr(var_state.reg++, 0, 0, 0, var_state.address_bits); > > the ++ is a hard to notice side-effect of the loop. It's cleaner to > separate it out or to have a for() loop for it. > > > > +static int __init mtrr_cleanup(unsigned address_bits) > > +{ > > + unsigned long i, base, size, def, dummy; > > + mtrr_type type; > > + struct res_range range[RANGE_NUM]; > > + int nr_range; > > + unsigned long extra_remove_base, extra_remove_size; > > try to use a 'christmas tree' ordering of variables, i.e.: > > > > + unsigned long extra_remove_base, extra_remove_size; > > > + unsigned long i, base, size, def, dummy; > > > + struct res_range range[RANGE_NUM]; > > + mtrr_type type; > > + int nr_range; > > > + return 1; > > + > > +} > > superfluous newline. > > all in one, this is a very useful and nice feature.
thanks. will submit a new one with fix.
YH
| |