Message-ID: <50994BC1.1000705@redhat.com>
Date:	Tue, 06 Nov 2012 09:41:21 -0800
From:	Josh Stone <jistone@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <anton@...hat.com>,
	David Smith <dsmith@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	"Suzuki K. Poulose" <suzuki@...ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: uprobes && pre-filtering

On 11/06/2012 09:02 AM, Oleg Nesterov wrote:
> On 11/06, Srikar Dronamraju wrote:
>> Another reason for having the filters in the current way was to have a
>> set of standard filters in uprobes code so that all users dont need to
>> recreate these filters.
> 
> IOW, you mean that both register_for_each_vma() and handler_chain() should
> use the same ->filter() method? Personally I do not think they should.
> 
> Because the semantics is different imo. register_for_each_vma() needs
> to know if we should modify mm and insert the breakpoint. handler_chain()
> just tries to skip ->handler() (and for no reason, imho).

I agree here - I don't see much use for filter(), and even if you have
it, the semantics are definitely different than prefilter().  You could
do all sorts of localized and temporal checks in filter() that make no
sense for a prefilter(), like check for a particular thread id, filter
every N hits, or filter only hits under calls to some foo().

[...]
>>> - Perhaps we should extend the API. We can add
>>>
>>> 	uprobe_apply(consumer, task, bool add_remove);
>>>
>>>   which adds/removes breakpoints to task->mm.
>>>
>>>   This way consumer can probe every task it wants to trace after
>>>   uprobe_register().
>>>
>>>   Its ->filter(UPROBE_FILTER_REGISTER) should simply return false. Or,
>>>   better, we can split uprobe_register() into 2 functions,
>>>   __uprobe_register() and uprobe_apply_all() which actually does
>>>   register_for_each_vma().
>>>
>>>   ***** QUESTION *****: perhaps this is all systemtap needs? ignoring
>>>   UPROBE_FILTER_MMAP.
>>>
>>> - Multiple consumers. uprobe_register/uprobe_unregister should be
>>>   modified so that register_for_each_vma() is called every time when
>>>   the new consumer comes or consumer goes away.
>>>
>>>   uprobe_apply(add_remove => false) should obviously consult other
>>>   consumers too.
>>
>>
>> So in this case, would uprobe_register() just add a consumer to a
>> new/existing uprobe. The actual probe insertion is done by the
>> uprobe_apply()/uprobe_apply_all().
> 
> Yes. Not sure we really need this, but to me this extension looks natural.
> 
> Frank, Josh, do you think it can help systemtap ?

Yes, I think this sounds closer to systemtap's model of probing.  We
already track tasks that come and go to see which are "interesting", so
we could easily call apply() at that time.  We actually watch mmaps too,
so I think we could apply() for that case as well.

We wouldn't even need filtering functions at all in this mode.  But
maybe other consumers could still use it, like perf.

However, it's not clear to me what value there is in uprobe_register, if
you always have to apply it too.  The modes are something like:

1. uprobe_register(); uprobe_apply_all();
2. uprobe_register(); uprobe_apply(); [...]

And the second could have a bunch of idle registrations waiting for the
first applicable task to come around.  So why not instead:

1. uprobe_register_all();
2. uprobe_register_task(); [...]

In this case, the second would have to allow the same consumer to be
repeated on different tasks, but it feels more natural to me.

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

Powered by blists - more mailing lists