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
--
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/
