On Mon, Aug 21, 2023 at 05:13:12PM +0800, Tong Tiangen wrote:
We found a softlock issue in our test, analyzed the logs, and found that
the relevant CPU call trace as follows:
CPU0:
_do_fork
-> copy_process()
-> write_lock_irq(&tasklist_lock) //Disable irq,waiting for
//tasklist_lock
CPU1:
wp_page_copy()
->pte_offset_map_lock()
-> spin_lock(&page->ptl); //Hold page->ptl
-> ptep_clear_flush()
-> flush_tlb_others() ...
-> smp_call_function_many()
-> arch_send_call_function_ipi_mask()
-> csd_lock_wait() //Waiting for other CPUs respond
//IPI
CPU2:
collect_procs_anon()
-> read_lock(&tasklist_lock) //Hold tasklist_lock
->for_each_process(tsk)
-> page_mapped_in_vma()
-> page_vma_mapped_walk()
-> map_pte()
->spin_lock(&page->ptl) //Waiting for page->ptl
We can see that CPU1 waiting for CPU0 respond IPI,CPU0 waiting for CPU2
unlock tasklist_lock, CPU2 waiting for CPU1 unlock page->ptl. As a result,
softlockup is triggered.
For collect_procs_anon(), we will not modify the tasklist, but only perform
read traversal. Therefore, we can use rcu lock instead of spin lock
tasklist_lock, from this, we can break the softlock chain above.
The only thing that's giving me pause is that there's no discussion
about why this is safe. "We're not modifying it" isn't really enough
to justify going from read_lock() to rcu_read_lock(). When you take a
normal read_lock(), writers are not permitted and so you see an atomic
snapshot of the list. With rcu_read_lock() you can see inconsistencies.
For example, if new tasks can be added to the tasklist, they may not
be seen by an iteration. Is this OK?
tasklist after they have been seen by the iteration. Is this OK?
As I understand the list RCU code, it guarantees that all tasks which
were on the list before rcu_read_lock() and remain on the list after
rcu_read_unlock() will be seen by a list iteration, while tasks which
are added or removed during that time may or may not be seen.
.