From mboxrd@z Thu Jan  1 00:00:00 1970
From: laijs@cn.fujitsu.com (Lai Jiangshan)
Date: Thu, 18 Dec 2008 14:07:42 +0800
Subject: [ltt-dev] [PATCH] ltt-channel: permanent eID
In-Reply-To: <20081218044259.GA22138@Krystal>
References: <4947AD15.5010801@cn.fujitsu.com> <20081217194017.GA6095@Krystal>
	<4949B3DE.5080605@cn.fujitsu.com> <20081218044259.GA22138@Krystal>
Message-ID: <4949E8AE.1010409@cn.fujitsu.com>

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> 
> Thanks for this description, comments below,
> 
>> changelog:
>>
>> Marker ID and its data must be permanent for text output, this patch
>> introduce permanent eID.
>>
>> 1) per channel permanent eID
>>
>> Basic struct for store eID's data is struct marker_entry. When a channel
>> is activated, all the struct marker_entry for this channel are permanent
>> until this channel is deactivated. In this period, event if the marker
>> is leaved or other things are happened, the eID is permanent.
>>
> 
> Ok, so you keep the struct marker_entry around until the channel is
> unused (all traces are destroyed, including that all in-kernel ascii
> converters have released the trace) ?

When "this channel is deactivated", the unused struct marker_entry for
this channel will be removed, event if this channel is still live.
all traces are destroyed means all channel are deactivated.

when we trace a channel, we say this channel is activated.

> 
>> This patch introduces 4 API for permanent eID:
>> int start_trace_channel(const char *channel); - activate a channel
>> int stop_trace_channel(const char *channel); - deactivate a channel
> 
> Hrm, can we change the names for
> get_trace_channel()
> put_trace_channel() ?
> 
> start and stop sounds a bit misleading with the tracing start/stop
> semantics.
> 
> Also, why do we have "waste" ids ? This is bad, because whenever
> possible, we should fit IDs in ~28 IDs allowed for "compact" event ids.
> If the IDs are larger than this, the tracer will have to generate an
> extended header and waste trace space. Do we really _need_ such id waste?

The previous code has a degression: when eID for a marker is changed,
userspace tool do not know the ID changed. this patch make eIDs are
not changed too frequent.

> 
>> const char *get_name_from_eid(const char *channel, u16 eID); - get name from eID
>> const char *get_fmt_from_eid(const char *channel, u16 eID); - get fmt from eID
>>
>> See ltt/ltt-tracer.c for the using of start/stop_trace_channel().
>>
> hrm, stop_trace_channel (or put_trace_channel) is in __destroy, but
> should probably be in the kref desctructor, so we are sure that it's
> only called with all references to the trace data, including the
> in-kernel ascii converters, have released their reference.
> 

start_trace_channel()
create ltt relay channel

ID's life time

destory ltt relay channel
stop_trace_channel()

Is it incorrect?

> 
> 
>> When a channel is deactivated(its trace_count come to 0), we will defrag eID
>> for this channel. AND we use "trace_mark(metadata, core_marker_id," to tell
>> userspace that id is reset.
> 
> Sounds good.
> 
>> 2) per channel struct marker_entry(permanent eID' data) management
>>
>> All struct marker_entry of a channel are threaded by struct list_head.
>>
>> It will help us remove a struct marker_entry quickly and help us manage
>> entries quickly(defrag_id(), remove_chanel_marker_entries())
>>
>> We also call marker_update_probes() in defrag_id().
>>
> 
> Hrm, aren't we taking the marker mutex twice here ? I am probably
> missing something.

Oh, I forgot marker_update_probes() needs marker.
This patch have been tested, but without marker_update_probes() in defrag_id().
It was added when I saw my review note about markers_compact_event_ids()
when I was going to send patch.
marker_update_probes() is affirmatively need after defrag_id() or
markers_compact_event_ids()(removed function).

> 
>> Since struct marker_entry is permanent eID' data. We move the definition
>> of struct marker_entry from marker.c to ltt-channel.h
>>
> 
> BTW, I would not use the term "permanent" for this. Because permanent,
> to me, really means "we allocate something that we will only free when
> the machine reboots". We are simply extending the life of the the eIDs
> for the duration of all references to those IDs.
> 
> Let's say that we extend the event ID lifetime to the overall event ID
> references, which now includes trace references, which includes ascii
> converters.
> 
>> 3) as previous code, we defrag channel index when all tracer is stopped.
>>
>> different: As described in 1),  permanent eID will defraged when a
>> channel is deactivated.
>>
>> It means this patch has more fine-grained ID management.
>>
> 
> There are two cases here :
> 
> Tracing starts
>   add/remove markers many times
> Tracing stops -> defrag
> 
> And
> 
> No tracing
> add/remove markers many times -> defrag each time
> 
> You don't say much about what is done at marker add/remove when tracing
> is inactive. Is there some compaction done ?

start/stop_trace_channel() is very short. Reviewing the code is better than
ugly English comment:

stop_trace_channel()
   remove_chanel_marker_entries() ------- remove unused marker_entry

start_trace_channel()
   defrag_id()     ----Usually, defrag_id() should in stop_trace_channel()
                 but delay defrag_id() until start_trace_channel() is better.

if no tracing
	removing marker_entry is done quickly. not defrag id

if tracing
	removing marker_entry will be delayed.