lkml.org 
[lkml]   [2013]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6)
    Date
    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, &param);
    > 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


    \
     
     \ /
      Last update: 2013-11-07 10:01    [W:8.746 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site