Re: [PATCH] tracing/uprobes: Support ftrace_event_file basemultibuffer

[Oleg Nesterov]

Hi,

Now that we finished the discussion of the similar code in kprobes,
let me summarize.

On 06/14, Oleg Nesterov wrote:
>
> On 06/14, zhangwei(Jovi) wrote:
> >
> > +		rcu_assign_pointer(tu->files, new);
> > +		tu->flags |= TP_FLAG_TRACE;
> > +
> > +		if (old) {
> > +			/* Make sure the probe is done with old files */
> > +			synchronize_sched();
> > +			kfree(old);
> > +		}
> > +	} else
> > +		tu->flags |= TP_FLAG_PROFILE;
>
> So it can set both TP_FLAG_TRACE and TP_FLAG_PROFILE, yes?
>
> If yes, this is not right. Until we change the pre-filtering at least.
> Currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive.

Yes. So please update this patch so that TP_FLAG_PROFILE can't be
set if TP_FLAG_TRACE is set and vice versa.

Once again, this limitation is artificial. But it was always here,
and we need more changes to remove it. I'll try to do this later.
(but if you want to play with this code - welcome ;)

Don't add the mutex, and do not use array-of-pointers (I hope you
noticed the recent discussion).

Locking. Oh, OK. Let it be rcu for now (but please do not forget
that you need rcu_read_lock, uprobe handlers run in the sleepable
context). This is sub-optimal, but this is just another indication
that uprobes API is not perfect, we can't use uprobe->register_sem.

Also. When I was reading trace_kprobes.c I notice the new (and imho
useful) feature, soft disable/enable. Perhaps you can make a 2nd
patch which adds the FTRACE_EVENT_FL_SOFT_DISABLED_BIT check?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/