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). 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. 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. > >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
