[ltt-dev] [PATCH 02/13] get name and fmt from permanent eID

[Lai Jiangshan]

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>> struct marker_entry is permanent, we introduce two APIs for
>> getting name and fmt from eIDs.
>>
>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
>> ---
>>  b/kernel/marker.c      |   34 ++++++++++++++++++++++++++++++++++
>>  include/linux/marker.h |    3 +++
>>  2 files changed, 37 insertions(+)
>> diff --git a/include/linux/marker.h b/include/linux/marker.h
>> index 32387e8..2fe8977 100644
>> --- a/include/linux/marker.h
>> +++ b/include/linux/marker.h
>> @@ -238,6 +238,9 @@ extern int marker_probe_unregister_private_data(marker_probe_func *probe,
>>  extern void *marker_get_private_data(const char *channel, const char *name,
>>  	marker_probe_func *probe, int num);
>>  
>> +const char *marker_get_name_form_id(u16 channel_id, u16 event_id);
>> +const char *marker_get_fmt_form_id(u16 channel_id, u16 event_id);
>> +
>>  /*
>>   * marker_synchronize_unregister must be called between the last marker probe
>>   * unregistration and the end of module exit to make sure there is no caller
>> diff --git a/kernel/marker.c b/kernel/marker.c
>> index b11a82a..6a918e2 100644
>> --- a/kernel/marker.c
>> +++ b/kernel/marker.c
>> @@ -997,6 +1010,40 @@ void *marker_get_private_data(const char *channel, const char *name,
>>  }
>>  EXPORT_SYMBOL_GPL(marker_get_private_data);
>>  
>> +static struct marker_entry *get_entry_from_id(u16 channel_id, u16 event_id)
>> +{
>> +	struct hlist_head *head;
>> +	struct hlist_node *node;
>> +	struct marker_entry *e, *found = NULL;
>> +	u32 hash = hash_32((channel_id << 16) | event_id, MARKER_HASH_BITS);
>> +
> 
> Can we add a define rather that hardcoding 16 in the c file ?

I think hardcode will good for checking by review when we haven't use
ltt_channel_id_t and ltt_event_id_t.

> 
>> +	mutex_lock(&markers_mutex);
>> +	head = id_table + hash;
>> +	hlist_for_each_entry(e, node, head, id_list) {
>> +		if (e->channel_id == channel_id && e->event_id == event_id) {
>> +			found = e;
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&markers_mutex);
>> +	return found;
> 
> Hrm, it would be good to add a comment to this function's header telling
> for what validity period we can safely use the marker_entry * returned
> because it is not protected by any mutex.
> 
> The idea is that the function is only called when tracing is active, and
> we insure that the marker_entry is kept there (permanent) until the
> trace ends. We are sure that we stopped using the marker entry before
> trace end if we make sure we wait for all references to the trace to be
> down to zero (that includes any bin-to-ascii trace output module which
> uses this get_entry_from_id()) before we consider the trace unused. Not
> only does that require a comment, but we should carefull check the code
> to make sure refcounting is ok.
> 

I think I can't write complex comment. (for my poor English)

> 
>> +}
>> +
>> +/* must call when ids/marker_entry are kept alive */
>> +const char *marker_get_name_form_id(u16 channel_id, u16 event_id)
>> +{
>> +	struct marker_entry *e = get_entry_from_id(channel_id, event_id);
>> +	return e ? e->name : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(marker_get_name_form_id);
>> +
> Same here.
> 
>> +const char *marker_get_fmt_form_id(u16 channel_id, u16 event_id)
>> +{
>> +	struct marker_entry *e = get_entry_from_id(channel_id, event_id);
>> +	return e ? e->format : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(marker_get_fmt_form_id);
>> +
> 
> And here.
> 
> Hrm, actually, it might be good to expose the symbol :
> 
> get_entry_from_id

It's not a good idea. struct marker_entry is private struct.

> 
> So we need only one hash table lookup to use both the name and fmt ?
> 
> Mathieu
> 
>>  /**
>>   * markers_compact_event_ids: Compact markers event IDs and reassign channels
>>   *
>>
>>
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>