Re: [PATCH 15/23] Drivers: hv: Use RCU to protect access of HK_TYPE_MANAGED_IRQ cpumask

On 4/21/26 11:09 PM, [email protected] wrote:
> Sashiko AI review found 3 potential issue(s):
> - [High] Infinite loop causing RCU stall when a NUMA node lacks housekeeping CPUs.
> - [Medium] Mixing `goto`-based cleanup with scope-based `guard()` violates subsystem guidelines.
> - [Low] Unnecessary unconditional memory allocation for non-performance channels.
> --
>
> [Note: Due to recitation restrictions, this report uses a free-form summary
> format rather than quoting the original patch lines directly.]
>
> commit bc4398753271c194097795d20b6095f594c74b4c
> Author: Waiman Long <[email protected]>
> Drivers: hv: Use RCU to protect access of HK_TYPE_MANAGED_IRQ cpumask
>
> This commit uses RCU to protect the HK_TYPE_MANAGED_IRQ cpumask during access,
> moving the cpumask allocation before taking the RCU read lock since the
> allocation can sleep.
>
> I noticed a few potential regressions in this patch:
>
> First, looking at the loop in init_vp_index(), if a selected NUMA node has
> no CPUs present in the housekeeping mask (for example, if the entire node is
> isolated), the intersection of available_mask and hk_mask becomes empty.
> The code then clears allocated_mask and jumps back to the retry label.
>
> Since the variables do not change state across retries, does this create
> an infinite loop? Because this patch places the loop inside an RCU
> read-side critical section via guard(rcu)(), would this spin result in
> an RCU stall?

Yes, it is possible that it becomes an infinite retry loop if the 
HK_TYPE_MANAGED_IRQ housekeeping cpumask does not contain any CPU in 
that particular node. I will post a patch in the next version to fix that.

>
> Second, the patch introduces guard(rcu)() in init_vp_index() while retaining
> the goto out_free cleanup path for free_cpumask_var(). According to the
> kernel's cleanup guidelines, mixing goto-based error handling with
> scope-based cleanups like guard() in the same function creates confusing
> ownership semantics.
Will remove the added goto instruction in the next version.
>
> Should this function be fully converted to scope-based cleanup, or
> alternatively, use explicit rcu_read_lock() and rcu_read_unlock() calls
> to avoid this mix?
>
> Finally, before this patch, the code used short-circuit logic to skip
> allocating the cpumask if the channel was not performance-critical. The
> patch refactors this to unconditionally call alloc_cpumask_var() at the top
> of the function before evaluating the channel type.
>
> Does this introduce unnecessary sleepable memory allocation and deallocation
> cycles for non-performance channels?

Will add the check for perf_chn before doing memory allocation in the 
next version. That should keep the current behavior.

Cheers,
Longman

>