RE: [PATCH] rcu/kvfree: Make page cache growing happen on the correct krcp

[Zhang, Qiang1]

> When invoke add_ptr_to_bulk_krc_lock() to queue ptr, will invoke
> krc_this_cpu_lock() return current CPU's krcp structure and get a
> bnode object from the krcp structure's ->bulk_head, if return is
> empty or the returned bnode object's nr_records is KVFREE_BULK_MAX_ENTR,
> when the can_alloc is set, will unlock current CPU's krcp->lock and
> allocate bnode, after that, will invoke krc_this_cpu_lock() again to
> return current CPU's krcp structure, if the CPU migration occurs,
> the krcp obtained at this time will not be consistent with the previous
> one, this causes the bnode will be added to the wrong krcp structure's
> ->bulk_head or trigger fill page work on wrong krcp.
> 
> This commit therefore re-hold krcp->lock after allocated page instead
> of re-call krc_this_cpu_lock() to ensure the consistency of krcp.
> 
> Signed-off-by: Zqiang <qiang1.zhang@xxxxxxxxx>
>
>Very good, thank you!  Queued for testing and further review, but
>please check my wordsmithing.


More clear and detailed description, Thanks Paul 😊.


>
>							Thanx, Paul
>
>------------------------------------------------------------------------
>
>commit a0bbb5785539ed846f4769368f24a296d54bc801
>Author: Zqiang <qiang1.zhang@xxxxxxxxx>
>Date:   Sat Apr 8 22:25:30 2023 +0800
>
>    rcu/kvfree: Use consistent krcp when growing kfree_rcu() page cache
>    
>    The add_ptr_to_bulk_krc_lock() function is invoked to allocate a new
>    kfree_rcu() page, also known as a kvfree_rcu_bulk_data structure.
>    The kfree_rcu_cpu structure's lock is used to protect this operation,
>    except that this lock must be momentarily dropped when allocating memory.
>    It is clearly important that the lock that is reacquired be the same
>    lock that was acquired initially via krc_this_cpu_lock().
>    
>    Unfortunately, this same krc_this_cpu_lock() function is used to
>    re-acquire this lock, and if the task migrated to some other CPU during
>    the memory allocation, this will result in the kvfree_rcu_bulk_data
>    structure being added to the wrong CPU's kfree_rcu_cpu structure.
>    
>    This commit therefore replaces that second call to krc_this_cpu_lock()
>    with raw_spin_lock_irqsave() in order to explicitly acquire the lock on
>    the correct kfree_rcu_cpu structure, thus keeping things straight even
>    when the task migrates.
>    
>    Signed-off-by: Zqiang <qiang1.zhang@xxxxxxxxx>
>    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
>
>diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>index 2699b7acf0e3..41daae3239b5 100644
>--- a/kernel/rcu/tree.c
>+++ b/kernel/rcu/tree.c
>@@ -3301,7 +3301,7 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp,
> 			// scenarios.
> 			bnode = (struct kvfree_rcu_bulk_data *)
> 				__get_free_page(GFP_KERNEL | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>-			*krcp = krc_this_cpu_lock(flags);
>+			raw_spin_lock_irqsave(&(*krcp)->lock, *flags);
> 		}
> 
> 		if (!bnode)