Messages in this thread |  | | From | Namhyung Kim <> | Subject | Re: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6) | Date | Thu, 07 Nov 2013 17:48:34 +0900 |
| |
On Wed, 6 Nov 2013 18:37:54 +0100, Oleg Nesterov wrote: > On 11/06, Namhyung Kim wrote: >> >> On Tue, 5 Nov 2013 20:24:01 +0100, Oleg Nesterov wrote: >> > On 11/05, Oleg Nesterov wrote: >> >> >> >> As for "-= tu->offset"... Can't we avoid it? User-space needs to calculate >> >> the "@" argument anyway, why it can't also substruct this offset? >> >> >> >> Or perhaps we can change parse_probe_arg("@") to update "param" ? Yes, >> >> in this case it needs another argument, not sure... >> > >> > Or, >> > >> >> + if (is_ret_probe(tu)) { >> >> + saved_ip = instruction_pointer(regs); >> >> + instruction_pointer_set(func); >> >> + } >> >> store_trace_args(...); >> >> + if (is_ret_probe(tu)) >> >> + instruction_pointer_set(saved_ip); >> > >> > we can put "-= tu->offset" here. >> >> I don't think I get the point. > > I meant, > > saved_ip = instruction_pointer(regs); > > // pass the "ip" which was used to calculate > // the @addr argument to fetch_*() methods > > temp_ip = is_ret_probe(tu) ? func : saved_ip; > temp_ip -= tu->offset; > instruction_pointer_set(temp_ip); > > store_trace_args(...); > > instruction_pointer_set(saved_ip); > > This way we can avoid the new "void *" argument for fetch_func_t, > we do not need it to calculate the address.
Okay, but as I said before, subtracting tu->offset part can be removed.
> > But: we still need the additional "bool translate_vaddr" to solve > the problems with FETCH_MTD_deref. > > We already discussed this a bit, previously I suggested the new > FETCH_MTD_memory_notranslate and > > - dprm->fetch = t->fetch[FETCH_MTD_memory]; > + dprm->fetch = t->fetch[FETCH_MTD_memory_notranslate]; > > change in parse_probe_arg().
Okay, I agree with you that adding one more fetch method will make things simpler.
> > However, now I think it would be more clean to leave FETCH_MTD_memory > alone and add FETCH_MTD_memory_dotranslate instead. > > So trace_uprobes.c should define > > void FETCH_FUNC_NAME(memory, type)(addr, ...) > { > copy_from_user((void __user *)addr); > } > > void FETCH_FUNC_NAME(memory_dotranslate, type)(addr, ...) > { > void __user *uaddr = get_user_vaddr(regs, addr); > copy_from_user(uaddr); > }
Looks good.
> > Then, > >> > Or. Perhaps we can leave "case '@'" in parse_probe_arg() and >> > FETCH_MTD_memory alone. You seem to agree that "absolute address" >> > can be useful anyway. >> >> Yes, but it's only meaningful to process-wide tracing sessions IMHO. > > Yes, yes, sure. > > I meant, we need both. Say, "perf probe "func global=@addr" means > FETCH_MTD_memory, and "perf probe "func global=*addr" means > FETCH_MTD_memory_dotranslate. > > Just in case, of course I do not care about the syntax, for example we > can use "@~addr" for translate (or not translate) or whatever.
Yeah, and I want to hear from Masami.
> > My only point: I think we need both to > > 1. avoid the new argument in fetch_func_t > > 2. allow the dump the data from the absolute address
I got it.
> > And just to simplify the discussion, lets assume we use "*addr" for > FETCH_MTD_memory_dotranslate and thus parse_probe_arg() gets the new > > case '*': > if (is_kprobe) > return -EINVAL; > > kstrtoul(arg + 1, 0, ¶m); > f->fn = t->fetch[FETCH_MTD_memory_dotranslate]; > f->data = (void *)param; > break; > > branch.
Looks good.
> >> > Instead, perhaps we can add FETCH_MTD_memory_do_fancy_addr_translation, >> > and, say, the new "case '*'" in parse_probe_arg() should add all the >> > neccessary info as f->data (like, say, FETCH_MTD_symbol). >> >> Could you elaborate this more? > > Yes, I was confusing sorry. > > As for FETCH_MTD_memory_do_fancy_addr_translation, please see above.
Okay.
> > As for "neccessary info as f->data". Suppose that we still have a reason > for the additional argument in FETCH_MTD_memory_dotranslate method. Even > in this case I don't think we should change the signature of fetch_func_t. > > What I think we can do is something like > > 1. Changed parse_probe_arg() to accept "struct trace_uprobe *tu" > instead of is_kprobe. Naturally, !tu can be used instead. > > 2. Introduce > > struct dotranslate_fetch_param { > struct trace_uprobe *tu; > fetch_func_t fetch; > fetch_func_t fetch_size; > }; > > 3. Change the "case '*'" above to do > > case '*': > if (!tu) > return -EINVAL; > > struct dotranslate_fetch_param *xxx = kmalloc(..); > > xxx->fetch = t->fetch[FETCH_MTD_memory]; > > // ... kstrtoul, fetch_size, etc, ... > > f->fn = t->fetch[FETCH_MTD_memory_dotranslate]; > f->data = (void *)xxx; > > 4. Update traceprobe_free_probe_arg/etc. > > 5. Now, > > void FETCH_FUNC_NAME(memory_dotranslate, type)(addr, ...) > { > struct dotranslate_fetch_param *xxx = data; > void __user *uaddr = get_user_vaddr(regs, addr, tu); > > xxx->fetch(regs, addr, dest); > } > > Yes, yes, I am sure I missed something and this is not that simple, > I am new to this "fetch" code. > > And even if I am right, let me repeat that I am not going to argue. > Well, at least too much ;) This looks better in my opinion, but this > is always subjective, so please free to ignore.
Thank you very much for providing good review, suggestion and pseudo code. :) I indeed like this approach too.
I'll change the code this way in next version.
Thanks, Namhyung
|  |