Commit ea0854170c95245a258b386c7a9314399c949fe0 added a fuction
update_end_of_memory_vars() to update high_memory, max_pfn and
max_low_pfn.
Here modified the function(added an argument to show add or remove).
And call it in arch_remove_memory() to update these variables too.
Signed-off-by: Zhang Zhen <[email protected]>
---
arch/x86/mm/init_64.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index df1a992..2557091 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -673,14 +673,24 @@ void __init paging_init(void)
* After memory hotplug the variables max_pfn, max_low_pfn and high_memory need
* updating.
*/
-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;
+ }
}
}
@@ -702,7 +712,7 @@ int arch_add_memory(int nid, u64 start, u64 size)
WARN_ON_ONCE(ret);
/* update max_pfn, max_low_pfn and high_memory */
- update_end_of_memory_vars(start, size);
+ update_end_of_memory_vars(start, size, true);
return ret;
}
@@ -1025,6 +1035,9 @@ int __ref arch_remove_memory(u64 start, u64 size)
ret = __remove_pages(zone, start_pfn, nr_pages);
WARN_ON_ONCE(ret);
+ /* update max_pfn, max_low_pfn and high_memory */
+ update_end_of_memory_vars(start, size, false);
+
return ret;
}
#endif
--
1.8.1.2
.
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()
?
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.
On 07/28/2014 04:12 PM, David Rientjes wrote:
> 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.
That does look better than my suggestion, generally.
It is broken in the remove case, though. In your example, the memory
being removed is assumed to be coming from the end of memory, and that
isn't always the case. I think you need something like:
if ((max_pfn >= start_pfn) && (max_pfn < end_pfn)
update_end_of_memory_vars(start);
But, yeah, that's a lot better than new functions.
On 2014/7/29 7:24, Dave Hansen wrote:
> On 07/28/2014 04:12 PM, David Rientjes wrote:
>> 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.
>
> That does look better than my suggestion, generally.
>
> It is broken in the remove case, though. In your example, the memory
> being removed is assumed to be coming from the end of memory, and that
> isn't always the case. I think you need something like:
>
> if ((max_pfn >= start_pfn) && (max_pfn < end_pfn)
> update_end_of_memory_vars(start);
>
> But, yeah, that's a lot better than new functions.
>
Thanks for your comments!
I will change according to your suggestions.
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
>