Re: [PATCH 0/8] uprobes: RCU-protected hot path optimizations

Liao, Chang
2024-08-08T07:51:19Z

在 2024/8/8 1:31, Andrii Nakryiko 写道:
> On Wed, Aug 7, 2024 at 10:11 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>
>> On 08/07, Andrii Nakryiko wrote:
>>>
>>> Yes, I was waiting for more of Peter's comments, but I guess I'll just
>>> send a v2 today.
>>
>> OK,
>>
>>> I'll probably include the SRCU+timeout logic for
>>> return_instances, and maybe lockless VMA parts as well.
>>
>> Well, feel free to do what you think right, but perhaps it would be
>> better to push this series first? at least 1-4.
>
> Ok, I can send those first 4 patches first and hopefully we can land
> them soon and move to the next part. I just also wrote up details
> about that crash in rb_find_rcu().
>
>>
>> As for lockless VMA. To me this needs more discussions. I didn't read
>
> We are still discussing, feel free to join the conversation.
>
>> your conversation with Peter and Suren carefully, but I too have some
>> concerns. Most probably I am wrong, and until I saw this thread I didn't
>> even know that vm_area_free() uses call_rcu() if CONFIG_PER_VMA_LOCK,
>> but still.
>>
>>>> As for 8/8 - I leave it to you and Peter. I'd prefer SRCU though ;)
>>>
>>> Honestly curious, why the preference?
>>
>> Well, you can safely ignore me, but since you have asked ;)
>>
>> I understand what SRCU does, and years ago I even understood (I hope)
>> the implementation. More or less the same for rcu_tasks. But as for
>> the _trace flavour, I simply fail to understand its semantics.
>
> Ok, I won't try to repeat Paul's explanations. If you are curious you
> can find them in comments to my previous batch register/unregister API
> patches.
>
>>
>>> BTW, while you are here :) What can you say about
>>> current->sighand->siglock use in handle_singlestep()?
>>
>> It should die, and this looks simple. I disagree with the patches
>> from Liao, see the
>> https://lore.kernel.org/all/20240801082407.1618451-1-liaochang1@huawei.com/
>> thread, but I agree with the intent.
>
> I wasn't aware of this patch, thanks for mentioning it. Strange that
> me or at least bpf@vger.kernel.org wasn't CC'ed.
>
> Liao, please cc bpf@ mailing list for future patches like that.

OK, sorry about that.

>
>>
>> IMO, we need a simple "bool restore_sigpending" in uprobe_task, it will make the
>> necessary changes really simple.
>

[...]

>>
>> (To clarify. In fact I think that a new TIF_ or even PF_ flag makes more sense,
>> afaics it can have more users. But I don't think that uprobes can provide enough
>> justification for that right now)

I also face the same choice when Oleg suggested me to add new flag to track the denied
flag, due to I haven't encountered scenarios outside of uprobe that would deny signal,
so I'm not confident of introduce new TIF_ flag without a fully understanding of potential
potential impacts.

>>
>> Oleg.
>>

--
BR
Liao, Chang