[lttng-dev] [PATCH lttng-ust 4/8] Add probe provider unregister function

[Francis Deslauriers]

2018-02-07 15:54 GMT-05:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>:
>
> ----- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote:
>
>> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com>
>> ---
>> include/lttng/ust-events.h  |  1 +
>> liblttng-ust/lttng-events.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 90 insertions(+)
>>
>> diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h
>> index caf7e63..019b0eb 100644
>> --- a/include/lttng/ust-events.h
>> +++ b/include/lttng/ust-events.h
>> @@ -656,6 +656,7 @@ void synchronize_trace(void);
>>
>> int lttng_probe_register(struct lttng_probe_desc *desc);
>> void lttng_probe_unregister(struct lttng_probe_desc *desc);
>> +void lttng_probe_provider_unregister_events(struct lttng_probe_desc *desc);
>> int lttng_fix_pending_events(void);
>> int lttng_probes_init(void);
>> void lttng_probes_exit(void);
>> diff --git a/liblttng-ust/lttng-events.c b/liblttng-ust/lttng-events.c
>> index 7419f78..e8d4857 100644
>> --- a/liblttng-ust/lttng-events.c
>> +++ b/liblttng-ust/lttng-events.c
>> @@ -789,6 +789,95 @@ void lttng_create_event_if_missing(struct lttng_enabler
>> *enabler)
>> }
>>
>> /*
>> + * Iterate over all the UST sessions to unregister and destroy all probes from
>> + * the probe provider descriptor passed in arguments. Must me called with the
>
> passed in arguments -> received as argument
>
>> + * ust_lock held.
>> + */
>> +void lttng_probe_provider_unregister_events(struct lttng_probe_desc
>> *provider_desc)
>> +{
>> +     int i;
>
> move int i; to the last variable (shortest line).
>
>> +     struct cds_list_head *sessionsp;
>> +     struct lttng_session *session;
>> +     struct cds_hlist_head *head;
>> +     struct cds_hlist_node *node;
>> +     struct lttng_event *event;
>> +
>> +     /* Get handle on list of sessions. */
>> +     sessionsp = _lttng_get_sessions();
>> +
>> +     /*
>> +      * Iterate over all events in the probe provider descriptions and sessions
>> +      * to queue the unregistration of the events.
>> +      */
>> +     for (i = 0; i < provider_desc->nr_events; i++) {
>> +             const char *event_name;
>> +             uint32_t hash;
>> +             size_t name_len;
>> +             const struct lttng_event_desc *event_desc;
>
> same here about var definition layout.
>
>> +
>> +             event_desc = provider_desc->event_desc[i];
>> +             event_name = event_desc->name;
>> +             name_len = strlen(event_name);
>> +             hash = jhash(event_name, name_len, 0);
>> +
>> +             /* Iterate over all session to find the current event description. */
>> +             cds_list_for_each_entry(session, sessionsp, node) {
>
> remove whiteline.
>
>> +
>> +                     /*
>> +                      * Get the list of events in the hashtable bucket and iterate to
>> +                      * find the event matching this descriptor.
>> +                      */
>> +                     head = &session->events_ht.table[hash & (LTTNG_UST_EVENT_HT_SIZE - 1)];
>> +                     cds_hlist_for_each_entry(event, node, head, hlist) {
>> +                             if (event_desc == event->desc) {
>> +
>> +                                     /* Queue the unregistration of this event. */
>> +                                     _lttng_event_unregister(event);
>> +                                     break;
>> +                             }
>> +                     }
>> +             }
>> +     }
>> +
>> +     /* Wait for grace period. */
>> +     synchronize_trace();
>> +     /* Prune the unregistration queue. */
>> +     __tracepoint_probe_prune_release_queue();
>> +
>> +     /*
>> +      * It is now safe to destroy the events and remove them from the event list
>> +      * and hashtables.
>> +      */
>> +     for (i = 0; i < provider_desc->nr_events; i++) {
>> +             const char *event_name;
>> +             uint32_t hash;
>> +             size_t name_len;
>> +             const struct lttng_event_desc *event_desc;
>
> same.
>
>> +
>> +             event_desc = provider_desc->event_desc[i];
>> +             event_name = event_desc->name;
>> +             name_len = strlen(event_name);
>> +             hash = jhash(event_name, name_len, 0);
>> +
>
>
>
>> +             /* Iterate over all sessions to find the current event description. */
>> +             cds_list_for_each_entry(session, sessionsp, node) {
>
> whiteline.
>
>> +
>> +                     /*
>> +                      * Get the list of events in the hashtable bucket and iterate to
>> +                      * find the event matching this descriptor.
>> +                      */
>
> beware for the iteration below: you may need to use a cds_list_for_each_entry_safe()
> if you can remove the list entry while you iterate.
Good point. Fixed.
>
>> +                     head = &session->events_ht.table[hash & (LTTNG_UST_EVENT_HT_SIZE - 1)];
>> +                     cds_hlist_for_each_entry(event, node, head, hlist) {
>> +                             if (event_desc == event->desc) {
>> +                                     _lttng_event_destroy(event);
>
> you have a "break" here. Is there a possibility that you can find more than one match ?

When this function is called, we are in the process of unloading a
probe provider, we iterate over
description of every events in that provider description. The if
statement above compares
the pointers of event descriptor so there is only one match possible
in a given process.
The break is there to avoid continuing the iteration over the other
events on that hlist needlessly.

All the coding style comments above were addressed.

Thank you,
Francis
>
> Thanks,
>
> Mathieu
>
>> +                                     break;
>> +                             }
>> +                     }
>> +             }
>> +     }
>> +}
>> +
>> +/*
>>  * Create events associated with an enabler (if not already present),
>>  * and add backward reference from the event to the enabler.
>>  */
>> --
>> 2.7.4
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com



-- 
Francis Deslauriers
Software developer
EfficiOS inc.