diff mbox series

[v2,3/4] rcu/kvfree: Move need_offload_krc() out of krcp->lock

Message ID 20221129155822.538434-4-urezki@gmail.com (mailing list archive)
State Accepted
Commit 11b8cb245f76cbf5b41a72a5eea2fd2fb31f533c
Headers show
Series kvfree_rcu() updates related to polled API | expand

Commit Message

Uladzislau Rezki Nov. 29, 2022, 3:58 p.m. UTC
Currently a need_offload_krc() function requires the krcp->lock
to be held because krcp->head can not be checked concurrently.

Fix it by updating the krcp->head using WRITE_ONCE() macro so
it becomes lock-free and safe for readers to see a valid data
without any locking.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 kernel/rcu/tree.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Paul E. McKenney Nov. 29, 2022, 11:38 p.m. UTC | #1
On Tue, Nov 29, 2022 at 04:58:21PM +0100, Uladzislau Rezki (Sony) wrote:
> Currently a need_offload_krc() function requires the krcp->lock
> to be held because krcp->head can not be checked concurrently.
> 
> Fix it by updating the krcp->head using WRITE_ONCE() macro so
> it becomes lock-free and safe for readers to see a valid data
> without any locking.

Don't we also need to use READ_ONCE() for the code loading this krcp->head
pointer?  Or do the remaining plain C-language accesses somehow avoid
running concurrently with those new WRITE_ONCE() invocations?

						Thanx, Paul

> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  kernel/rcu/tree.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 445f8c11a9a3..c94c17194299 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3058,7 +3058,7 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  			// objects queued on the linked list.
>  			if (!krwp->head_free) {
>  				krwp->head_free = krcp->head;
> -				krcp->head = NULL;
> +				WRITE_ONCE(krcp->head, NULL);
>  			}
>  
>  			WRITE_ONCE(krcp->count, 0);
> @@ -3072,6 +3072,8 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  		}
>  	}
>  
> +	raw_spin_unlock_irqrestore(&krcp->lock, flags);
> +
>  	// If there is nothing to detach, it means that our job is
>  	// successfully done here. In case of having at least one
>  	// of the channels that is still busy we should rearm the
> @@ -3079,8 +3081,6 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  	// still in progress.
>  	if (need_offload_krc(krcp))
>  		schedule_delayed_monitor_work(krcp);
> -
> -	raw_spin_unlock_irqrestore(&krcp->lock, flags);
>  }
>  
>  static enum hrtimer_restart
> @@ -3250,7 +3250,7 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
>  
>  		head->func = ptr;
>  		head->next = krcp->head;
> -		krcp->head = head;
> +		WRITE_ONCE(krcp->head, head);
>  		success = true;
>  	}
>  
> @@ -3327,15 +3327,12 @@ static struct shrinker kfree_rcu_shrinker = {
>  void __init kfree_rcu_scheduler_running(void)
>  {
>  	int cpu;
> -	unsigned long flags;
>  
>  	for_each_possible_cpu(cpu) {
>  		struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
>  
> -		raw_spin_lock_irqsave(&krcp->lock, flags);
>  		if (need_offload_krc(krcp))
>  			schedule_delayed_monitor_work(krcp);
> -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
>  	}
>  }
>  
> -- 
> 2.30.2
>
Uladzislau Rezki Nov. 30, 2022, 12:56 p.m. UTC | #2
On Tue, Nov 29, 2022 at 03:38:33PM -0800, Paul E. McKenney wrote:
> On Tue, Nov 29, 2022 at 04:58:21PM +0100, Uladzislau Rezki (Sony) wrote:
> > Currently a need_offload_krc() function requires the krcp->lock
> > to be held because krcp->head can not be checked concurrently.
> > 
> > Fix it by updating the krcp->head using WRITE_ONCE() macro so
> > it becomes lock-free and safe for readers to see a valid data
> > without any locking.
> 
> Don't we also need to use READ_ONCE() for the code loading this krcp->head
> pointer?  Or do the remaining plain C-language accesses somehow avoid
> running concurrently with those new WRITE_ONCE() invocations?
>
It can be concurrent. I was thinking about it. For some reason i decided
to keep readers as a "regular" ones for loading the krcp->head.

In this case it might take time for readers to see an updated value
as a worst case scenario.

So i need to update it or upload one more patch on top of v2. Should
i upload a new patch?

Thanks!

--
Uladzislau Rezki
Paul E. McKenney Nov. 30, 2022, 6:44 p.m. UTC | #3
On Wed, Nov 30, 2022 at 01:56:17PM +0100, Uladzislau Rezki wrote:
> On Tue, Nov 29, 2022 at 03:38:33PM -0800, Paul E. McKenney wrote:
> > On Tue, Nov 29, 2022 at 04:58:21PM +0100, Uladzislau Rezki (Sony) wrote:
> > > Currently a need_offload_krc() function requires the krcp->lock
> > > to be held because krcp->head can not be checked concurrently.
> > > 
> > > Fix it by updating the krcp->head using WRITE_ONCE() macro so
> > > it becomes lock-free and safe for readers to see a valid data
> > > without any locking.
> > 
> > Don't we also need to use READ_ONCE() for the code loading this krcp->head
> > pointer?  Or do the remaining plain C-language accesses somehow avoid
> > running concurrently with those new WRITE_ONCE() invocations?
> >
> It can be concurrent. I was thinking about it. For some reason i decided
> to keep readers as a "regular" ones for loading the krcp->head.
> 
> In this case it might take time for readers to see an updated value
> as a worst case scenario.
> 
> So i need to update it or upload one more patch on top of v2. Should
> i upload a new patch?

Sending an additional patch should be fine.  Unless you would rather it
be folded into one of the existing patches, in which case please start
with the set that I have queued.

							Thanx, Paul
Uladzislau Rezki Dec. 2, 2022, 1:19 p.m. UTC | #4
On Wed, Nov 30, 2022 at 10:44:55AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 30, 2022 at 01:56:17PM +0100, Uladzislau Rezki wrote:
> > On Tue, Nov 29, 2022 at 03:38:33PM -0800, Paul E. McKenney wrote:
> > > On Tue, Nov 29, 2022 at 04:58:21PM +0100, Uladzislau Rezki (Sony) wrote:
> > > > Currently a need_offload_krc() function requires the krcp->lock
> > > > to be held because krcp->head can not be checked concurrently.
> > > > 
> > > > Fix it by updating the krcp->head using WRITE_ONCE() macro so
> > > > it becomes lock-free and safe for readers to see a valid data
> > > > without any locking.
> > > 
> > > Don't we also need to use READ_ONCE() for the code loading this krcp->head
> > > pointer?  Or do the remaining plain C-language accesses somehow avoid
> > > running concurrently with those new WRITE_ONCE() invocations?
> > >
> > It can be concurrent. I was thinking about it. For some reason i decided
> > to keep readers as a "regular" ones for loading the krcp->head.
> > 
> > In this case it might take time for readers to see an updated value
> > as a worst case scenario.
> > 
> > So i need to update it or upload one more patch on top of v2. Should
> > i upload a new patch?
> 
> Sending an additional patch should be fine.  Unless you would rather it
> be folded into one of the existing patches, in which case please start
> with the set that I have queued.
> 
Done.

--
Uladzislau Rezki
diff mbox series

Patch

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 445f8c11a9a3..c94c17194299 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3058,7 +3058,7 @@  static void kfree_rcu_monitor(struct work_struct *work)
 			// objects queued on the linked list.
 			if (!krwp->head_free) {
 				krwp->head_free = krcp->head;
-				krcp->head = NULL;
+				WRITE_ONCE(krcp->head, NULL);
 			}
 
 			WRITE_ONCE(krcp->count, 0);
@@ -3072,6 +3072,8 @@  static void kfree_rcu_monitor(struct work_struct *work)
 		}
 	}
 
+	raw_spin_unlock_irqrestore(&krcp->lock, flags);
+
 	// If there is nothing to detach, it means that our job is
 	// successfully done here. In case of having at least one
 	// of the channels that is still busy we should rearm the
@@ -3079,8 +3081,6 @@  static void kfree_rcu_monitor(struct work_struct *work)
 	// still in progress.
 	if (need_offload_krc(krcp))
 		schedule_delayed_monitor_work(krcp);
-
-	raw_spin_unlock_irqrestore(&krcp->lock, flags);
 }
 
 static enum hrtimer_restart
@@ -3250,7 +3250,7 @@  void kvfree_call_rcu(struct rcu_head *head, void *ptr)
 
 		head->func = ptr;
 		head->next = krcp->head;
-		krcp->head = head;
+		WRITE_ONCE(krcp->head, head);
 		success = true;
 	}
 
@@ -3327,15 +3327,12 @@  static struct shrinker kfree_rcu_shrinker = {
 void __init kfree_rcu_scheduler_running(void)
 {
 	int cpu;
-	unsigned long flags;
 
 	for_each_possible_cpu(cpu) {
 		struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
 
-		raw_spin_lock_irqsave(&krcp->lock, flags);
 		if (need_offload_krc(krcp))
 			schedule_delayed_monitor_work(krcp);
-		raw_spin_unlock_irqrestore(&krcp->lock, flags);
 	}
 }