[v1] rv: Simplify task monitor slot management

[PATCH] rv: Simplify task monitor slot management

Posted by liqiang 1 week, 4 days ago

The slot array already tracks allocation and task_monitor_count
duplicates that state. On an invalid second release, the old code
warns but still decrements the counter, corrupting later allocations.

Use the slot array as the sole source of truth. Return after warning
about an unused slot, and return -EBUSY when no slot is free.

Signed-off-by: liqiang <liqiang01@kylinos.cn>
---
 kernel/trace/rv/rv.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index ee4e68102f17..187d87d5991c 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -164,7 +164,6 @@ struct dentry *get_monitors_root(void)
  */
 LIST_HEAD(rv_monitors_list);
 
-static int task_monitor_count;
 static bool task_monitor_slots[CONFIG_RV_PER_TASK_MONITORS];
 
 int rv_get_task_monitor_slot(void)
@@ -173,21 +172,14 @@ int rv_get_task_monitor_slot(void)
 
 	lockdep_assert_held(&rv_interface_lock);
 
-	if (task_monitor_count == CONFIG_RV_PER_TASK_MONITORS)
-		return -EBUSY;
-
-	task_monitor_count++;
-
 	for (i = 0; i < CONFIG_RV_PER_TASK_MONITORS; i++) {
-		if (task_monitor_slots[i] == false) {
+		if (!task_monitor_slots[i]) {
 			task_monitor_slots[i] = true;
 			return i;
 		}
 	}
 
-	WARN_ONCE(1, "RV task_monitor_count and slots are out of sync\n");
-
-	return -EINVAL;
+	return -EBUSY;
 }
 
 void rv_put_task_monitor_slot(int slot)
@@ -199,10 +191,10 @@ void rv_put_task_monitor_slot(int slot)
 		return;
 	}
 
-	WARN_ONCE(!task_monitor_slots[slot], "RV releasing unused task_monitor_slots: %d\n",
-		  slot);
+	if (WARN_ONCE(!task_monitor_slots[slot],
+		      "RV releasing unused task monitor slot: %d\n", slot))
+		return;
 
-	task_monitor_count--;
 	task_monitor_slots[slot] = false;
 }
 
-- 
2.43.0

Re: [PATCH] rv: Simplify task monitor slot management

Posted by Gabriele Monaco 1 week, 4 days ago

On Tue, 2026-07-14 at 16:33 +0800, liqiang wrote:
> The slot array already tracks allocation and task_monitor_count
> duplicates that state. On an invalid second release, the old code
> warns but still decrements the counter, corrupting later allocations.
> 
> Use the slot array as the sole source of truth. Return after warning
> about an unused slot, and return -EBUSY when no slot is free.
> 
> Signed-off-by: liqiang <liqiang01@kylinos.cn>

Thanks for the patch! It looks good to me:

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>

However, your signoff should include a "known identity" [1]. For most people
this is an official name and surname (Li Qiang ?).

You could probably just reply with the new signoff, but it's probably better you
send a V2 with it updated. Feel free to also include my review tag.

I suggest you configure git accordingly e.g.:

  git config set user.name "Name Surname"
  git commit --amend --reset-author --signoff
  # will open an editor from there remove the old signoff and add the review tag

Thanks,
Gabriele

[1] -
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> ---
>  kernel/trace/rv/rv.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index ee4e68102f17..187d87d5991c 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -164,7 +164,6 @@ struct dentry *get_monitors_root(void)
>   */
>  LIST_HEAD(rv_monitors_list);
>  
> -static int task_monitor_count;
>  static bool task_monitor_slots[CONFIG_RV_PER_TASK_MONITORS];
>  
>  int rv_get_task_monitor_slot(void)
> @@ -173,21 +172,14 @@ int rv_get_task_monitor_slot(void)
>  
>  	lockdep_assert_held(&rv_interface_lock);
>  
> -	if (task_monitor_count == CONFIG_RV_PER_TASK_MONITORS)
> -		return -EBUSY;
> -
> -	task_monitor_count++;
> -
>  	for (i = 0; i < CONFIG_RV_PER_TASK_MONITORS; i++) {
> -		if (task_monitor_slots[i] == false) {
> +		if (!task_monitor_slots[i]) {
>  			task_monitor_slots[i] = true;
>  			return i;
>  		}
>  	}
>  
> -	WARN_ONCE(1, "RV task_monitor_count and slots are out of sync\n");
> -
> -	return -EINVAL;
> +	return -EBUSY;
>  }
>  
>  void rv_put_task_monitor_slot(int slot)
> @@ -199,10 +191,10 @@ void rv_put_task_monitor_slot(int slot)
>  		return;
>  	}
>  
> -	WARN_ONCE(!task_monitor_slots[slot], "RV releasing unused
> task_monitor_slots: %d\n",
> -		  slot);
> +	if (WARN_ONCE(!task_monitor_slots[slot],
> +		      "RV releasing unused task monitor slot: %d\n", slot))
> +		return;
>  
> -	task_monitor_count--;
>  	task_monitor_slots[slot] = false;
>  }
>

[PATCH v2] rv: Simplify task monitor slot management

Posted by Li Qiang 1 week, 3 days ago

The slot array already tracks allocation and task_monitor_count
duplicates that state. On an invalid second release, the old code
warns but still decrements the counter, corrupting later allocations.

Use the slot array as the sole source of truth. Return after warning
about an unused slot, and return -EBUSY when no slot is free.

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 kernel/trace/rv/rv.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index ee4e68102f17..187d87d5991c 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -164,7 +164,6 @@ struct dentry *get_monitors_root(void)
  */
 LIST_HEAD(rv_monitors_list);
 
-static int task_monitor_count;
 static bool task_monitor_slots[CONFIG_RV_PER_TASK_MONITORS];
 
 int rv_get_task_monitor_slot(void)
@@ -173,21 +172,14 @@ int rv_get_task_monitor_slot(void)
 
 	lockdep_assert_held(&rv_interface_lock);
 
-	if (task_monitor_count == CONFIG_RV_PER_TASK_MONITORS)
-		return -EBUSY;
-
-	task_monitor_count++;
-
 	for (i = 0; i < CONFIG_RV_PER_TASK_MONITORS; i++) {
-		if (task_monitor_slots[i] == false) {
+		if (!task_monitor_slots[i]) {
 			task_monitor_slots[i] = true;
 			return i;
 		}
 	}
 
-	WARN_ONCE(1, "RV task_monitor_count and slots are out of sync\n");
-
-	return -EINVAL;
+	return -EBUSY;
 }
 
 void rv_put_task_monitor_slot(int slot)
@@ -199,10 +191,10 @@ void rv_put_task_monitor_slot(int slot)
 		return;
 	}
 
-	WARN_ONCE(!task_monitor_slots[slot], "RV releasing unused task_monitor_slots: %d\n",
-		  slot);
+	if (WARN_ONCE(!task_monitor_slots[slot],
+		      "RV releasing unused task monitor slot: %d\n", slot))
+		return;
 
-	task_monitor_count--;
 	task_monitor_slots[slot] = false;
 }
 
-- 
2.43.0