Skip to contentSkip to conversation

sched/fair: Care divide error in update_task_scan_period()

PATCH Superseded Yasuaki Ishimatsu 4 participants 7 messages Oct 8, 2014 – Oct 9, 2014 Scheduler

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Yasuaki Ishimatsu

author
#0 lore raw

PATCH 1/1 sched/fair: Care divide error in update_task_scan_period()

While offling node by hot removing memory, the following divide error occurs:

divide error: 0000 [#1] SMP
[...]

  Call Trace:
   [...] handle_mm_fault
   [...] ? try_to_wake_up
   [...] ? wake_up_state
   [...] __do_page_fault
   [...] ? do_futex
   [...] ? put_prev_entity
   [...] ? __switch_to
   [...] do_page_fault
   [...] page_fault
  [...]
  RIP  [<ffffffff810a7081>] task_numa_fault
   RSP <ffff88084eb2bcb0>

The issue occurs as follows:
1. When page fault occurs and page is allocated from node 1, task_struct->numa_faults_buffer_memory[] of node 1 is incremented and p->numa_faults_locality[] is also incremented as follows:

o numa_faults_buffer_memory[] o numa_faults_locality[]
NR_NUMA_HINT_FAULT_TYPES
| 0 | 1 |
---------------------------------- ----------------------
node 0 | 0 | 0 | remote | 0 |
node 1 | 0 | 1 | locale | 1 |
---------------------------------- ----------------------

2. node 1 is offlined by hot removing memory.

3. When page fault occurs, fault_types[] is calculated by using

     p->numa_faults_buffer_memory[] of all online nodes in
     task_numa_placement(). But node 1 was offline by step 2. So
     the fault_types[] is calculated by using only
     p->numa_faults_buffer_memory[] of node 0. So both of fault_types[]
     are set to 0.

4. The values(0) of fault_types[] pass to update_task_scan_period().

5. numa_faults_locality[1] is set to 1. So the following division is

     calculated.

        static void update_task_scan_period(struct task_struct *p,
                                unsigned long shared, unsigned long private){
        ...
                ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
        }

6. But both of private and shared are set to 0. So divide error occurs here.

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

Signed-off-by Yasuaki Ishimatsu <[email protected]>
---
 kernel/sched/fair.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
kernel/sched/fair.c +19 -11
index bfa3c86..fb7dc3f 100644
@@ -1496,18 +1496,26 @@ static void update_task_scan_period(struct task_struct *p,
 			slot = 1;
 		diff = slot * period_slot;
 	} else {
-		diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
+		if (unlikely((private + shared) == 0))
+			/*
+			 * This is a rare case. The trigger is node offline.
+			 */
+			diff = 0;
+		else {
+			diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
 
-		/*
-		 * Scale scan rate increases based on sharing. There is an
-		 * inverse relationship between the degree of sharing and
-		 * the adjustment made to the scanning period. Broadly
-		 * speaking the intent is that there is little point
-		 * scanning faster if shared accesses dominate as it may
-		 * simply bounce migrations uselessly
-		 */
-		ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
-		diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+			/*
+			 * Scale scan rate increases based on sharing. There is
+			 * an inverse relationship between the degree of sharing
+			 * and the adjustment made to the scanning period.
+			 * Broadly speaking the intent is that there is little
+			 * point scanning faster if shared accesses dominate as
+			 * it may simply bounce migrations uselessly
+			 */
+			ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS,
+							(private + shared));
+			diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+		}
 	}
 
 	p->numa_scan_period = clamp(p->numa_scan_period + diff,
Signature · 1 line
1.8.3.1
maintainer
#1 lore raw
Quoted 32 lines · Yasuaki Ishimatsu
kernel/sched/fair.c +12 -11
index bfa3c86..fb7dc3f 100644
@@ -1496,18 +1496,26 @@ static void update_task_scan_period(struct task_struct *p,
 			slot = 1;
 		diff = slot * period_slot;
 	} else {
-		diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
+		if (unlikely((private + shared) == 0))
+			/*
+			 * This is a rare case. The trigger is node offline.
+			 */
+			diff = 0;
+		else {
+			diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
 
-		/*
-		 * Scale scan rate increases based on sharing. There is an
-		 * inverse relationship between the degree of sharing and
-		 * the adjustment made to the scanning period. Broadly
-		 * speaking the intent is that there is little point
-		 * scanning faster if shared accesses dominate as it may
-		 * simply bounce migrations uselessly
-		 */
-		ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
-		diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+			/*
+			 * Scale scan rate increases based on sharing. There is
+			 * an inverse relationship between the degree of sharing
+			 * and the adjustment made to the scanning period.
+			 * Broadly speaking the intent is that there is little

kernel/sched/fair.c static void update_task_scan_period(struct task_struct *p, · line 1521

Quoted 10 lines
kernel/sched/fair.c +7 -0
+			 * point scanning faster if shared accesses dominate as
+			 * it may simply bounce migrations uselessly
+			 */
+			ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS,
+							(private + shared));
+			diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+		}
 	}
 
 	p->numa_scan_period = clamp(p->numa_scan_period + diff,

Yeah, so I don't like the patch nor do I really like the function as it stands -- which I suppose is part of why I don't like the patch.

The problem I have with the function is that its very inconsistent in behaviour. In the early return path it sets numa_scan_period and numa_next_scan, in the later return path it sets numa_scan_period and numa_faults_locality.

I feel both return paths should affect the same set of variables, esp. the non clearing of numa_faults_locality in the early path seems weird.

The thing I suppose I don't like about the patch is its added indentation and the fact that the simple +1 thing wasn't considered.

#2 lore raw
In reply to Yasuaki Ishimatsu
Quoted 95 lines · Yasuaki Ishimatsu

While offling node by hot removing memory, the following divide error occurs:

divide error: 0000 [#1] SMP
[...]

  Call Trace:
   [...] handle_mm_fault
   [...] ? try_to_wake_up
   [...] ? wake_up_state
   [...] __do_page_fault
   [...] ? do_futex
   [...] ? put_prev_entity
   [...] ? __switch_to
   [...] do_page_fault
   [...] page_fault
  [...]
  RIP  [<ffffffff810a7081>] task_numa_fault
   RSP <ffff88084eb2bcb0>

The issue occurs as follows:
1. When page fault occurs and page is allocated from node 1, task_struct->numa_faults_buffer_memory[] of node 1 is incremented and p->numa_faults_locality[] is also incremented as follows:

o numa_faults_buffer_memory[] o numa_faults_locality[]
NR_NUMA_HINT_FAULT_TYPES
| 0 | 1 |
---------------------------------- ----------------------
node 0 | 0 | 0 | remote | 0 |
node 1 | 0 | 1 | locale | 1 |
---------------------------------- ----------------------

2. node 1 is offlined by hot removing memory.

3. When page fault occurs, fault_types[] is calculated by using

     p->numa_faults_buffer_memory[] of all online nodes in
     task_numa_placement(). But node 1 was offline by step 2. So
     the fault_types[] is calculated by using only
     p->numa_faults_buffer_memory[] of node 0. So both of fault_types[]
     are set to 0.

4. The values(0) of fault_types[] pass to update_task_scan_period().

5. numa_faults_locality[1] is set to 1. So the following division is

     calculated.

        static void update_task_scan_period(struct task_struct *p,
                                unsigned long shared, unsigned long private){
        ...
                ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
        }

6. But both of private and shared are set to 0. So divide error occurs here.

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

Signed-off-by Yasuaki Ishimatsu <[email protected]>
---
 kernel/sched/fair.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
kernel/sched/fair.c +10 -11
index bfa3c86..fb7dc3f 100644
@@ -1496,18 +1496,26 @@ static void update_task_scan_period(struct task_struct *p,
 			slot = 1;
 		diff = slot * period_slot;
 	} else {
-		diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
+		if (unlikely((private + shared) == 0))
+			/*
+			 * This is a rare case. The trigger is node offline.
+			 */
+			diff = 0;
+		else {
+			diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
 
-		/*
-		 * Scale scan rate increases based on sharing. There is an
-		 * inverse relationship between the degree of sharing and
-		 * the adjustment made to the scanning period. Broadly
-		 * speaking the intent is that there is little point
-		 * scanning faster if shared accesses dominate as it may
-		 * simply bounce migrations uselessly
-		 */
-		ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
-		diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+			/*
+			 * Scale scan rate increases based on sharing. There is
+			 * an inverse relationship between the degree of sharing

kernel/sched/fair.c static void update_task_scan_period(struct task_struct *p, · line 1519

Quoted 10 lines
kernel/sched/fair.c +9 -0
+			 * and the adjustment made to the scanning period.
+			 * Broadly speaking the intent is that there is little
+			 * point scanning faster if shared accesses dominate as
+			 * it may simply bounce migrations uselessly
+			 */
+			ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS,
+							(private + shared));
+			diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+		}
 	}

How about just

ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared + 1));

Regards,
Wanpeng Li

Quoted 1 line
kernel/sched/fair.c +0 -0
 	p->numa_scan_period = clamp(p->numa_scan_period + diff,
#4 lore raw
In reply to Yasuaki Ishimatsu
Quoted 3 lines · Yasuaki Ishimatsu

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

How about a simple

    if (private + shared) == 0)
          return;

higher up in the function, to avoid adding an extra layer of indentation and confusion to the main part of the function?

maintainer
#5 lore raw
Quoted 6 lines · Rik van Riel
Quoted 3 lines · Yasuaki Ishimatsu

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

Quoted 8 lines

How about a simple

    if (private + shared) == 0)
          return;

higher up in the function, to avoid adding an extra layer of indentation and confusion to the main part of the function?

At which point we'll have 3 different return semantics. Should we not clear numa_faults_localityp[], even in this case?

author
#6 lore raw
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Rik van Riel
Quoted 3 lines · Yasuaki Ishimatsu

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

How about a simple

     if (private + shared) == 0)
           return;
Quoted 7 lines
Quoted 3 lines

higher up in the function, to avoid adding an extra layer of indentation and confusion to the main part of the function?

At which point we'll have 3 different return semantics. Should we not clear numa_faults_localityp[], even in this case?

I'm not familiar with Numa balancing feature. So I want to know it too. If it's not necessary to clear numa_faults_locality[], I'll apply the idea.

Thanks,
Yasuaki Ishimatsu

author
#3 lore raw
In reply to Wanpeng Li
Quoted 111 lines · Wanpeng Li
Quoted 95 lines · Yasuaki Ishimatsu

While offling node by hot removing memory, the following divide error occurs:

   divide error: 0000 [#1] SMP
   [...]
   Call Trace:
    [...] handle_mm_fault
    [...] ? try_to_wake_up
    [...] ? wake_up_state
    [...] __do_page_fault
    [...] ? do_futex
    [...] ? put_prev_entity
    [...] ? __switch_to
    [...] do_page_fault
    [...] page_fault
   [...]
   RIP  [<ffffffff810a7081>] task_numa_fault
    RSP <ffff88084eb2bcb0>

The issue occurs as follows:
1. When page fault occurs and page is allocated from node 1, task_struct->numa_faults_buffer_memory[] of node 1 is incremented and p->numa_faults_locality[] is also incremented as follows:

o numa_faults_buffer_memory[] o numa_faults_locality[]
NR_NUMA_HINT_FAULT_TYPES
| 0 | 1 |
---------------------------------- ----------------------
node 0 | 0 | 0 | remote | 0 |
node 1 | 0 | 1 | locale | 1 |
---------------------------------- ----------------------

2. node 1 is offlined by hot removing memory.

3. When page fault occurs, fault_types[] is calculated by using

      p->numa_faults_buffer_memory[] of all online nodes in
      task_numa_placement(). But node 1 was offline by step 2. So
      the fault_types[] is calculated by using only
      p->numa_faults_buffer_memory[] of node 0. So both of fault_types[]
      are set to 0.

4. The values(0) of fault_types[] pass to update_task_scan_period().

5. numa_faults_locality[1] is set to 1. So the following division is

      calculated.

         static void update_task_scan_period(struct task_struct *p,
                                 unsigned long shared, unsigned long private){
         ...
                 ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
         }

6. But both of private and shared are set to 0. So divide error occurs here.

The divide error is rare case because the trigger is node offline. By this patch, when both of private and shared are set to 0, diff is just set to 0, not calculating the division.

Signed-off-by Yasuaki Ishimatsu <[email protected]>
---
  kernel/sched/fair.c | 30 +++++++++++++++++++-----------
  1 file changed, 19 insertions(+), 11 deletions(-)
kernel/sched/fair.c +10 -11
index bfa3c86..fb7dc3f 100644
@@ -1496,18 +1496,26 @@ static void update_task_scan_period(struct task_struct *p,
  			slot = 1;
  		diff = slot * period_slot;
  	} else {
-		diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
+		if (unlikely((private + shared) == 0))
+			/*
+			 * This is a rare case. The trigger is node offline.
+			 */
+			diff = 0;
+		else {
+			diff = -(NUMA_PERIOD_THRESHOLD - ratio) * period_slot;
 
-		/*
-		 * Scale scan rate increases based on sharing. There is an
-		 * inverse relationship between the degree of sharing and
-		 * the adjustment made to the scanning period. Broadly
-		 * speaking the intent is that there is little point
-		 * scanning faster if shared accesses dominate as it may
-		 * simply bounce migrations uselessly
-		 */
-		ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared));
-		diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+			/*
+			 * Scale scan rate increases based on sharing. There is
+			 * an inverse relationship between the degree of sharing

kernel/sched/fair.c static void update_task_scan_period(struct task_struct *p, · line 1519

Quoted 10 lines
kernel/sched/fair.c +9 -0
+			 * and the adjustment made to the scanning period.
+			 * Broadly speaking the intent is that there is little
+			 * point scanning faster if shared accesses dominate as
+			 * it may simply bounce migrations uselessly
+			 */
+			ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS,
+							(private + shared));
+			diff = (diff * ratio) / NUMA_PERIOD_SLOTS;
+		}
  	}

How about just

Quoted 1 line

ratio = DIV_ROUND_UP(private * NUMA_PERIOD_SLOTS, (private + shared + 1));

Thank you for providing sample code. Rik also provided other idea. So I am confirming which is better idea.

Thanks,
Yasuaki Ishimatsu

Quoted 7 lines

Regards,
Wanpeng Li

Quoted 1 line
kernel/sched/fair.c +0 -0
  	p->numa_scan_period = clamp(p->numa_scan_period + diff,