[lkml]   [2013]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 16/17] uprobes: Allocate ->utask before handler_chain() for tracing handlers
(2013/12/13 4:46), Oleg Nesterov wrote:
> On 12/12, Masami Hiramatsu wrote:
>> (2013/12/12 3:11), Oleg Nesterov wrote:
>>> On 12/11, Masami Hiramatsu wrote:
>>>> But it could skip the handler_chain silently. It could confuse users
>>>> why their probe doesn't hit as expected.
>>> No, we will restart the same (probed) instruction, handle_swbp()
>>> will be called again, get_utask() will be called again.
>> Hmm, in that case, how would you avoid infinite recursive loop??
> Masami, I do not understand your concerns ;) see below.
>> Would you repeat it until get_utask() != NULL?
> Yes, the task will loop until kmalloc(GFP_KERNEL) succeeds, and I see
> nothing wrong here.
> Just in case, let me remind that it won't loop in kernel mode, it can
> take a signal, it can be killed. And it is not recursive, this is
> like restart after page fault (which btw can fault again if the page
> was unmapped again, and "in theory" this loop can be infinite too).

Ah! I got it :)

> And why this is bad? Once again, this is GFP_KERNEL allocation, if it
> loops "indefinitely" there is something wrong. Even a single GFP_KERNEL
> failure likely means the task is already killed by oom, so it will
> simply exit when it returns to user-mode.

Indeed. It should be killed.

> And how this differs from, say, the "endless" should_alloc_retry() loop
> in __alloc_pages_slowpath() ? And note that in this case we loop in
> kernel mode. Of course this is not possible "in practice", but the same
> is true for the "endless" loop you are worried about.

Agreed, at least that is not uprobe's business :)

>>>> Hmm, in that case, should uprobes handlers never be called on ppc with
>>>> this change?
>>> Why? With this change ppc will have ->utask != NULL even if it doesn't
>>> need it at all.
>> Ah, I see. This changes that.
> Yes, this is why the changelog says "a bit unfortunate", we allocate the
> memory even there is no trace_uprobe consumer. So it would be nice to
> cleanup this later somehow, but imho this is a low priority problem and
> perhaps we will simply postulate that uprobe_consumer->handler() can
> rely on task->utask != NULL and remove get_utask() from pre_ssout().
> The only necessary cleanup (in my opinion) is that we should add another
> member into the union in uprobe_task for trace_uprobe.c, but again I
> think we should do this later to avoid the (potentially conflicting)
> changes in this series.
> Oleg.

Thank you,

IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

 \ /
  Last update: 2013-12-13 03:21    [W:0.036 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site