Re: [PATCH v8 5/6] Drivers: hv: Add refcount and locking to mem regions

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 12/3/2025 11:55 AM, Stanislav Kinsburskii wrote:
> On Wed, Dec 03, 2025 at 11:26:23AM -0800, Nuno Das Neves wrote:
>> On 12/3/2025 10:24 AM, Stanislav Kinsburskii wrote:
>>> Introduce kref-based reference counting and spinlock protection for
>>> memory regions in Hyper-V partition management. This change improves
>>> memory region lifecycle management and ensures thread-safe access to the
>>> region list.
>>>
>>> Also improves the check for overlapped memory regions during region
>>> creation, preventing duplicate or conflicting mappings.
>>>
>>> Previously, the regions list was protected by the partition mutex.
>>> However, this approach is too heavy for frequent fault and invalidation
>>> operations. Finer grained locking is now used to improve efficiency and
>>> concurrency.
>>>
>>> This is a precursor to supporting movable memory regions. Fault and
>>> invalidation handling for movable regions will require safe traversal of
>>> the region list and holding a region reference while performing
>>> invalidation or fault operations.
>>>
>>> Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>>> ---
>>>  drivers/hv/mshv_regions.c   |   19 ++++++++++++++++---
>>>  drivers/hv/mshv_root.h      |    6 +++++-
>>>  drivers/hv/mshv_root_main.c |   32 ++++++++++++++++++++++++--------
>>>  3 files changed, 45 insertions(+), 12 deletions(-)
>>>
>>> @@ -1657,8 +1670,10 @@ static void destroy_partition(struct mshv_partition *partition)
>>>  	remove_partition(partition);
>>>  
>>>  	hlist_for_each_entry_safe(region, n, &partition->pt_mem_regions,
>>> -				  hnode)
>>> -		mshv_region_destroy(region);
>>> +				  hnode) {
>>> +		hlist_del(&region->hnode);
>>> +		mshv_region_put(region);
>>> +	}
>>>  
>>
>> With the following patch introducing movable memory, it looks like the
>> list could be traversed by mshv_partition_region_by_gfn() even while
>> the this hlist_del() is being called.
>>
>> Maybe that's not possible for some reason I'm unaware of, could you
>> explain why we don't need to spin_lock here for hlist_del()?
>> Or, alternatively, use hlist_for_each_entry_safe() in
>> mshv_partition_region_by_gfn() to guard against the deletion?
>>
> 
> This function (destroy_partition) is called when there are no active
> references for neither partition not its VPs (they are destroyed in the
> same function above).
> In other words, there can't be any callers for mshv_partition_region_by_gfn.

Ah, I see, even if the mmu_notifier is still active, it doesn't traverse the
list, as it gets the region by container_of().
Thanks.

> 
> As per mshv_partition_region_by_gfn function itself, the caller is
> supposed to take the lock.
> 
> Giving it more thought, I'm strating to think that rw lock her ewould be
> a better option than a spinlock + reference count, as regions won't be
> added or remove to often and using it would allow to get rid of
> reference counting.
> 
> However, this looks like an optimization that isn't required an its
> usefulness can be investigated in future.
> 
> Thanks,
> Stanislav
> 
>>>  	/* Withdraw and free all pages we deposited */
>>>  	hv_call_withdraw_memory(U64_MAX, NUMA_NO_NODE, partition->pt_id);
>>> @@ -1856,6 +1871,7 @@ mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
>>>  
>>>  	INIT_HLIST_HEAD(&partition->pt_devices);
>>>  
>>> +	spin_lock_init(&partition->pt_mem_regions_lock);
>>>  	INIT_HLIST_HEAD(&partition->pt_mem_regions);
>>>  
>>>  	mshv_eventfd_init(partition);
>>>
>>>