[ltt-dev] [PATCH] ltt-channel: permanent eID

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> 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.
>

OK

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

An event ID should _never_ be re-used for a different marker when a trace
is active. But when tracing is activated, we defragment the ID space (as
you do). Actually, we could postpone the ID allocation to tracing
activation rather than doing it at marker registration.

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

This is correct, but the "destroy ltt relay channel" does not happen
exactly in

        for (i = 0; i < trace->nr_channels; i++) {
                chan = &trace->channels[i];
                if (chan->active)
                        trace->ops->remove_channel(chan);
        }

Because all it calls is :

static void ltt_relay_remove_channel(struct ltt_channel_struct *channel)
{
        struct rchan *rchan = channel->trans_channel_data;

        ltt_relay_close(rchan);
          -> does a kref_put(&buf->kref, relay_remove_buf);
        kref_put(&channel->kref, ltt_relay_release_channel);
}


But relay_file_open (in ltt-relay-alloc.c) does :
  kref_get(&buf->kref);
And relay_file_release in ltt-relay-alloc.c does :
  kref_put(&buf->kref, relay_remove_buf);

__ltt_trace_destroy
  trace->ops->remove_channel -> ltt-relay.c:ltt_relay_remove_channel
    ltt-relay-alloc.c:ltt_relay_close
      ltt-relay-alloc.c:relay_close_buf
        kref_put(&buf->kref, relay_remove_buf);
 
So we basically have to :
- Have __ltt_trace_destroy releasing the kref
- Have any file opener releasing the kref

Before the following gets called :

ltt-relay-alloc.c:relay_remove_buf
  buf->chan->cb->remove_buf_file -> ltt-relay.c:ltt_remove_buf_file_callback
    ltt_relay_destroy_buffer
      kref_put(&ltt_chan->trace->ltt_transport_kref,
        ltt_release_transport);
      kref_put(&ltt_chan->kref, ltt_relay_release_channel);
      kref_put(&trace->kref, ltt_release_trace);
  relay_destroy_buf
    kref_put(&chan->kref, relay_destroy_channel);



If we look below in __ltt_trace_destroy, we see :

       /*
         * Wait for lttd readers to release the files, therefore making sure
         * the last subbuffers have been read.
         */
        if (atomic_read(&trace->kref.refcount) > 1) {
                int ret = 0;
                __wait_event_interruptible(trace->kref_wq,
                        (atomic_read(&trace->kref.refcount) == 1), ret);
        }
        kref_put(&trace->kref, ltt_release_trace);

This lets us wait for any file opener to exit before we continue.

So, the correct moment to do the stop_trace_channel(chan->channel_name);
is when all references to the traces are done. The easiest spot to put
it is in ltt_release_trace() just before the ltt_channels_trace_free
call.



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

Yep.

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

That makes sense.

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68