Re: [PATCH] uprobes: Improve the usage of xol slots for better scalability

["Liao, Chang" <liaochang1@huawei.com>]

在 2024/9/18 20:25, Andi Kleen 写道:
> Liao Chang <liaochang1@huawei.com> writes:
>> +
>> +/*
>> + * xol_recycle_insn_slot - recycle a slot from the garbage collection list.
>> + */
>> +static int xol_recycle_insn_slot(struct xol_area *area)
>> +{
>> +	struct uprobe_task *utask;
>> +	int slot = UINSNS_PER_PAGE;
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(utask, &area->gc_list, gc) {
>> +		/*
>> +		 * The utask associated slot is in-use or recycling when
>> +		 * utask associated slot_ref is not one.
>> +		 */
>> +		if (test_and_put_task_slot(utask)) {
>> +			slot = utask->insn_slot;
>> +			utask->insn_slot = UINSNS_PER_PAGE;
>> +			clear_bit(slot, area->bitmap);
>> +			atomic_dec(&area->slot_count);
>> +			get_task_slot(utask);
> 
> Doesn't this need some annotation to make ThreadSanitizer happy?

Hi, Andi

Sorry, I know nothing about the ThreadSanitizer and related annotation,
could you provide some information about it, thanks.

> Would be good to have some commentary why doing so
> many write operations with merely a rcu_read_lock as protection is safe.
> It might be safer to put some write type operations under a real lock. 
> Also it is unclear how the RCU grace period for utasks is enforced.

You are right, but I think using atomic refcount routine might be a more
suitable apprach for this scenario. The slot_ret field of utask instance
is used to track the status of insn_slot. slot_ret supports three values.
A value of 2 means the utask associated insn_slot is currently in use by
uprobe. A value of 1 means the slot is no being used by uprobe. A value
of 0 means the slot has been reclaimed. So in some term, the atomic refcount
routine test_and_pout_task_slot() also avoid the racing when writing to
the utask instance, providing additional status information about insn_slot.

BTW, You reminded me that since it might recycle the slot after deleting the
utask from the garbage collection list, so it's necessary to use
test_and_put_task_slot() to avoid the racing on the stale utask. the correct
code might be something like this:

@@ -1771,16 +1783,16 @@ static void xol_free_insn_slot(struct task_struct *tsk)

        spin_lock_irqsave(&area->list_lock, flags);
        list_del_rcu(&tsk->utask->gc);
+       /* Ensure the slot is not in use or reclaimed on other CPU */
+       if (test_and_put_task_slot(tsk->utask)) {
+               clear_bit(tsk->utask->insn_slot, area->bitmap);
+               atomic_dec(&area->slot_count);
+               tsk->utask->insn_slot = UINSNS_PER_PAGE;
+               get_task_slot(tsk->utask);
+       }
        spin_unlock_irqrestore(&area->list_lock, flags);
        synchronize_rcu();

If I've made any mistakes about RCU usage, please don't hesitate to corret me.
Thank you in advance.

> 
> 
> -Andi
> 

-- 
BR
Liao, Chang