Message-ID: <49092AEB.2080000@cn.fujitsu.com>
Date:	Thu, 30 Oct 2008 11:32:59 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	sadump@...fujitsu.com
CC:	ltt-dev@...ts.casi.polymtl.ca,
	"Martin J. Bligh" <mbligh@...igh.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	kernel-trace-list@...hat.com, linux-kernel@...r.kernel.org,
	systemtap@...rces.redhat.com
Subject: Re: [sadump 04308] Re: [ltt-dev] LTTng 0.44 and LTTV 0.11.3

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs@...fujitsu.com) wrote:
>> Mathieu Desnoyers wrote:
>>> - I have also vastly simplified locking in the markers and tracepoints
>>>   by using _only_ the modules mutex. I actually took this mutex out of
>>>   module.c and created its own file so tracepoints and markers can use
>>>   it. That should please Lai Jiangshan. Although he may have some work
>>>   to do to see how his new probes manager might benefit from it.
>>>
>>>   See :
>>>   http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=7aea87ac46df7613d68034f5904bc8d575069076
>>>   and
>>>   http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=5f6814237f7a67650e7b6214d916825e3f8fc1b7
>>>   http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=410ba66a1cbe27a611e1c18c0a53e87b4652a2c9
>>>
>> Hi, Mathieu,
>>
>> I strongly reject for removing tracepoint_mutex and marker_mutex.
>>
>> As an independent subsystem, we should use our own locks. Do not use others.
>> otherwise coupling will be increased in linux kernel.
>> I condemn unnecessary coupling.
>>
>> Our tracepoint & marker had tied to modules(for traveling all tracepoints
>> or markers). The best thing is that we do not increase the coupling.
>>
>> [PATCH 2/2] tracepoint: introduce *_noupdate APIs.
>> is helpful for auto-active-tracepoint-mechanism.
>>
>> 		Thanx, Lai.
>>
> 
> Hi Lai,
> 
> The approach you propose looks interesting. Please see below to make
> sure we are on the same page.
> 
> The problem is that when we want to connect
> markers/tracepoints/immediate values together, it results in a real
> locking mess between
> 
> modules_mutex
>   markers_mutex
>     tracepoints_mutex
>       imv_mutex
> 
> When we want to take care of a marker at module load, we have to insure
> the following calling scenario is correct :
> 
> load_module()
>   call markers_update_probes_range() (on the module markers)
>     call tracepoint register (to automatically enable a tracepoint
>                                when a marker is connected to it)
>       call tracepoints_update_probe_range (on kernel core and all modules)
>         call imv_update_range (on kernel core and all modules)
> 
> The current locking status of tracepoints vs markers does not currently
> allow tracepoints_register to be called from the marker update because
> it would take the modules_mutex twice.
> 
> What you propose is something like this :
> 
> load_module()
>   call markers_update_probes_range()
>     call tracepoint_register_noupdate (to automatically enable a tracepoint
>                                        when a marker is connected to it)
>   call tracepoints_update_all() (for core kernel and all modules (*))
>     name##__imv = (i)
>   call imv_update_all() (for core kernel and all modules (*))
> 
> (*) This is required because registering a tracepoint might have impact
>     outside of the module in which the marker is located. Same for
>     changing an immediate value.

Er, my patch cannot handle updates when load_module().

> 
> And on marker_register_probe() :
>   call markers_update_probes_range()
>     call tracepoint_register_noupdate
>   call tracepoints_update_all()
>     name##__imv = (i)
>   call imv_update_all()
> 
> Which basically uses the same trick I used for immediate values : it
> separates the "backing data" update (name##_imv = (i)) from the actual
> update that needs to iterate on the modules.
> 
> The only thing we have to be aware of is that it actually couples
> markers/tracepoints/immediate values much more thightly to keep separate
> locking for each, because, as the example above shows, the markers have
> to be aware that they must call tracepoints_update_all and
> imv_update_all explicitely. On the plus side, it requires much less
> iterations on the module sections, which is a clear win.
> 
> So the expected mutex nesting order is (indent implies "nested in"):
> 
> On load_module :
> 
> modules_mutex
>   markers_mutex
>   tracepoints_mutex
>   imv_mutex
> 
> On marker register :
> 
> markers_mutex
>   tracepoints_mutex
>   imv_mutex
> 
> On tracepoint register :
> 
> tracepoints_mutex
>   imv_mutex
> 
> On imv_update :
> 
> imv_mutex
> 
> So yes, I think your approach is good, although there are some
> implementation quirks in the patch you submitted. I'll comment by
> replying to your other post.

Hmm, right, the patch <<[PATCH 2/2] tracepoint: introduce *_noupdate APIs>>
is quirks for it separate working into 2 steps.

currently, markers and tracepoint abuse RCU and use RCU in a very ugly way.
patches <<[PATCH 1/2] tracepoint: simplify for tracepoint using RCU>>
and <<[PATCH tip/tracing/markers] new probes manager>> had toll us how ugly they
are. if you do not remove tracepoints_mutex and markers_mutex, these two
patches can be applied now. (and if you want to remove mutexs, I will changed
these two patches a little.)

--------------------

Actually, markers and tracepoint are not like immediate-value, we do not need
update markers and tracepoint on load_module(), we can register_module_notifier()
and update them when MODULE_STATE_COMING.

[Quick Quiz 1] why imv_update_all() must called on load_module()?
load_module() will setup parameters, the routine of setuping parameters
will use immediate-value.

sys_init_module:
  load_module
    setup parameters
  blocking_notifier_call_chain(MODULE_STATE_COMING)
  mod->init();

the markers and tracepoint in "setup parameters" will not be actived by this 
approach, it's OK for it's a trace tool.

PS and OT: why not introduce module_param_imv?
a little like this:
#define module_param_imv(name, type, perm)				\
	module_param_named(name, name##__imv, type, perm)
and update them in MODULE_STATE_COMING.

most module param are readonly after initialized.

----------------------------
if you do not like <<[PATCH 2/2] tracepoint: introduce *_noupdate APIs>>,
here is the second way:
introduce these five APIs in module.c
module_iter_start()   - require module_mutex and return first module
module_iter_next()
module_iter_stop()    - release module_mutex
__module_iter_start() - do not require module_mutex
__module_iter_stop()

When we have fixed the mutex mess, code changed by this approach are
the least. and we can implement robust tracepoint_iter, marker_iter
by using these APIs.

------------------------------

I strongly reject for removing tracepoint_mutex and marker_mutex.
you will pay for it when markers and tracepoint become powerful
after you remove tracepoint_mutex and marker_mutex. and the
modules guys will condemn you for bring coupling for modules.

	Thanx, Lai.

> 
> Thanks,
> 
> Mathieu
> 
> 
>>> So hopefully everyone will be happy with this new release. :)
>>>
>>> Mathieu
>>>
>>
>> --
>> 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/
>>
> 


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