On Thursday, June 7, 2007 12:45 am Eric W. Biederman wrote: > Ok. Overall this feels good but a few nits below. > Would it make sense to split this into two patches. > The first to just do the cleanup that removes the allocations > for holding the mttr ranges?
I suppose we could split it, but it's small, and the only reason for removing the allocations was so that we could init it earlier. > > struct mtrr_state { > > - struct mtrr_var_range *var_ranges; > > + struct mtrr_var_range var_ranges[NUM_VAR_RANGES]; > > Could we name it MAX_VAR_RANGES and not NUM_VAR_RANGES. > In practices this is going to be 8 for every cpu I know of, > so calling this NUM_VAR_RANGES may be a little confusing. You're right, I should have kept the old name with MAX_ in it. I'll fix it up. > > /* RED-PEN: this is accessed without any locking */ > > -extern unsigned int *usage_table; > > +extern unsigned int usage_table[]; > > I think that should be: > > +extern unsigned int usage_table[NUM_VAR_RANGES]; > > Or even better yet the declaration moved to a header file. Oops, yeah, this should just be in mtrr.h. > This looks like it will handle the common case, so I have no major > objections to this code. > > At least in theory and possibly in practice there are a couple of > corner cases we have missed her. > > - Overlapping MTRRs. Overlapping should be ok, since that's usually intentional (e.g. one big wb range with a portion of uc space due to another mtrr). > - What happens if we have uncached memory lower down? Holes definitely aren't dealt with, but then we haven't seen any yet... > Except for performance problems I guess that case is relatively > harmless. - Is it possible and worth it to amend the e820 map, so it > shows the problem area as Reserved or otherwise not usable RAM? That would be useful, but only if we moved the check to a little earlier, prior to the addition of the active ranges from the e820. Might be a little nicer than adjusting end_pfn, but will ultimately achieve the same thing... Jesse - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/