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

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

Oleg,

My bad to take so long to reply. I have recently returned from a long vacation.

在 2024/9/28 1:18, Oleg Nesterov 写道:
> On 09/27, Liao Chang wrote:
>>
>> +int recycle_utask_slot(struct uprobe_task *utask, struct xol_area *area)
>> +{
>> +	int slot = UINSNS_PER_PAGE;
>> +
>> +	/*
>> +	 * Ensure that the slot is not in use on other CPU. However, this
>> +	 * check is unnecessary when called in the context of an exiting
>> +	 * thread. See xol_free_insn_slot() called from uprobe_free_utask()
>> +	 * for more details.
>> +	 */
>> +	if (test_and_put_task_slot(utask)) {
>> +		list_del(&utask->gc);
>> +		clear_bit(utask->insn_slot, area->bitmap);
>> +		atomic_dec(&area->slot_count);
>> +		utask->insn_slot = UINSNS_PER_PAGE;
>> +		refcount_set(&utask->slot_ref, 1);
> 
> This lacks a barrier, CPU can reorder the last 2 insns
> 
> 		refcount_set(&utask->slot_ref, 1);
> 		utask->insn_slot = UINSNS_PER_PAGE;
> 
> so the "utask->insn_slot == UINSNS_PER_PAGE" check in xol_get_insn_slot()
> can be false negative.

Good catcha! Would an atomic_set() with release ordering be sufficient here
instead of smp_mb()?

> 
>> +static unsigned long xol_get_insn_slot(struct uprobe_task *utask,
>> +				       struct uprobe *uprobe)
>>  {
>>  	struct xol_area *area;
>>  	unsigned long xol_vaddr;
>> @@ -1665,16 +1740,46 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
>>  	if (!area)
>>  		return 0;
>>
>> -	xol_vaddr = xol_take_insn_slot(area);
>> -	if (unlikely(!xol_vaddr))
>> +	/*
>> +	 * The racing on the utask associated slot_ref can occur unless the
>> +	 * area runs out of slots. This isn't a common case. Even if it does
>> +	 * happen, the scalability bottleneck will shift to another point.
>> +	 */
> 
> I don't understand the comment, I guess it means the race with
> recycle_utask_slot() above.
> >> +	if (!test_and_get_task_slot(utask))

Exactly, While introducing another refcount operation here might seem like
a downside, the potential racing on it should be less than the ones on
xol_area->bitmap and xol_area->slot_count(which you've already optimized).

>>  		return 0;
> 
> No, we can't do this. xol_get_insn_slot() should never fail.
> 
> OK, OK, currently xol_get_insn_slot() _can_ fail, but only if get_xol_area()
> fails to allocate the memory. Which should "never" happen and we can do nothing
> in this case anyway.

Sorry, I haven't trace the exact path where xol_get_insn_slot() fails. I suspect
it might repeatedly trigger BRK exceptions before get_xol_area() successfully
returns. Please correct me if I am wrong.

> 
> But it certainly must not fail if it races with another thread, this is insane.

Agreed, it is somewhat costly when race fails. I suggest that it allocates a new
slot upon the race fails instead of returning 0.

> 
> And. This patch changes the functions which ask for cleanups. I'll try to send
> a couple of simple patches on Monday.

Thank you for pointing that out, I must have missed some patches while I was on
vacation, I will carefully review the mailing list to ensure that this patch can
work with any recent cleanups.

> 
> Oleg.
> 
> 

-- 
BR
Liao, Chang