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

[Andi Kleen <ak@linux.intel.com>]

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

Documentation/dev-tools/kcsan.rst

> > 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

Does it really all need to be lockless? Perhaps you can only make the 
common case lockless, but then only when the list overflows take a lock 
and avoid a lot of races. That might be good enough for performance.

If you really want a complex lockless scheme you need very clear documentation
in comments and commit logs at least.

Also there should be a test case that stresses the various cases.

I would just use a lock 
> 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);
> +       }

I would have expected you would add a if (racing) bail out, assume the
other CPU will do the work type check but that doesn't seem to be what
the code is doing.

-Andi