On Mon, 9 Apr 2018 13:59:16 +0530
Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Masami,
>
> On 04/09/2018 12:58 PM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Wed, 4 Apr 2018 14:01:10 +0530
> > Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> wrote:
> >
> >> @@ -2054,15 +2060,21 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> >> }
> >>
> >> /* Use the tp->address for uprobes */
> >> - if (tev->uprobes)
> >> + if (tev->uprobes) {
> >> err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> >> - else if (!strncmp(tp->symbol, "0x", 2))
> >> + if (uprobe_ref_ctr_is_supported() &&
> >> + tp->ref_ctr_offset &&
> >> + err >= 0)
> >> + err = strbuf_addf(&buf, "(0x%lx)", tp->ref_ctr_offset);
> > If the kernel doesn't support uprobe_ref_ctr but the event requires
> > to increment uprobe_ref_ctr, I think we should (at least) warn user here.
>
> pr_debug("A semaphore is associated with %s:%s and seems your kernel doesn't support it.\n"
> tev->group, tev->event);
>
> Looks good?
I think it should be pr_warning() and return NULL, since user may not be able to
trace the event even if it is enabled.
>
> >> @@ -776,14 +784,21 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> >> {
> >> struct strbuf buf;
> >> char *ret = NULL, **args;
> >> - int i, args_count;
> >> + int i, args_count, err;
> >> + unsigned long long ref_ctr_offset;
> >>
> >> if (strbuf_init(&buf, 32) < 0)
> >> return NULL;
> >>
> >> - if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> - sdtgrp, note->name, pathname,
> >> - sdt_note__get_addr(note)) < 0)
> >> + err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> >> + sdtgrp, note->name, pathname,
> >> + sdt_note__get_addr(note));
> >> +
> >> + ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
> >> + if (uprobe_ref_ctr_is_supported() && ref_ctr_offset && err >= 0)
> >> + err = strbuf_addf(&buf, "(0x%llx)", ref_ctr_offset);
> > We don't have to care about uprobe_ref_ctr support here, because
> > this information will be just cached, not directly written to
> > uprobe_events.
>
> Sure, will remove the check.
Thanks!
>
> Thanks for the review :).
> Ravi
>
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
