(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, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
