On Mon, 28 Jul 2014, Dave Hansen wrote:
> On 07/28/2014 05:32 AM, Zhang Zhen wrote:
> > -static void update_end_of_memory_vars(u64 start, u64 size)
> > +static void update_end_of_memory_vars(u64 start, u64 size, bool flag)
> > {
> > - unsigned long end_pfn = PFN_UP(start + size);
> > -
> > - if (end_pfn > max_pfn) {
> > - max_pfn = end_pfn;
> > - max_low_pfn = end_pfn;
> > - high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
> > + unsigned long end_pfn;
> > +
> > + if (flag) {
> > + end_pfn = PFN_UP(start + size);
> > + if (end_pfn > max_pfn) {
> > + max_pfn = end_pfn;
> > + max_low_pfn = end_pfn;
> > + high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
> > + }
> > + } else {
> > + end_pfn = PFN_UP(start);
> > + if (end_pfn < max_pfn) {
> > + max_pfn = end_pfn;
> > + max_low_pfn = end_pfn;
> > + high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
> > + }
> > }
> > }
>
> I would really prefer not to see code like this.
>
> This patch takes a small function that did one thing, copies-and-pastes
> its code 100%, subtly changes it, and makes it do two things. The only
> thing to tell us what the difference between these two subtly different
> things is a variable called 'flag'. So the variable is useless in
> trying to figure out what each version is supposed to do.
>
> But, this fixes a pretty glaring deficiency in the memory remove code.
>
> I would suggest making two functions. Make it clear that one is to be
> used at remove time and the other at add time. Maybe
>
> move_end_of_memory_vars_down()
> and
> move_end_of_memory_vars_up()
>
I agree, but I'm not sure the suggestion is any better than the patch. I
think it would be better to just figure out whether anything needs to be
updated in the caller and then call a generic function.
So in arch_add_memory(), do
end_pfn = PFN_UP(start + size);
if (end_pfn > max_pfn)
update_end_of_memory_vars(end_pfn);
and in arch_remove_memory(),
end_pfn = PFN_UP(start);
if (end_pfn < max_pfn)
update_end_of_memory_vars(end_pfn);
and then update_end_of_memory_vars() becomes a three-liner.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
