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.
> +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))
> 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.
But it certainly must not fail if it races with another thread, this is insane.
And. This patch changes the functions which ask for cleanups. I'll try to send
a couple of simple patches on Monday.
Oleg.
