[lttng-dev] [PATCH lttng-tools] UST periodical metadata flush

[David Goulet]

Mathieu Desnoyers:
>>>>  
>>>> -	/* Inifinite blocking call, waiting for transmission */
>>>> +	/* Infinite blocking call, waiting for transmission */
>>>>  restart_poll:
>>>> -	health_poll_entry();
>>>> -	ret = lttng_poll_wait(&events, -1);
>>>> -	health_poll_exit();
>>>> -	if (ret < 0) {
>>>> -		/*
>>>> -		 * Restart interrupted system call.
>>>> -		 */
>>>> -		if (errno == EINTR) {
>>>> -			goto restart_poll;
>>>> +	while (1) {
>>>
>>> why are we getting this refactoring ? Is it necessary ?
>>
>> It is. We've added a metadata socket that receives commands from the
>> consumer so we need to poll at vitam eternam for that.
> 
> OK. we just need to review this code very carefully. I see it's mainly
> an indentation change, so hopefully the change is not too intrusive.

It is. The main part was simply the else if on the pollfd to check for
event on the metadata socket.

[...]
>>>>   * For a given application and session, push metadata to consumer. The session
>>>>   * lock MUST be acquired here before calling this.
>>>> + * Either sock or consumer is required : if sock is NULL, the default
>>>> + * socket to send the metadata is retrieved from consumer, if sock
>>>> + * is not NULL we use it to send the metadata.
>>>>   *
>>>>   * Return 0 on success else a negative error.
>>>>   */
>>>>  static int push_metadata(struct ust_registry_session *registry,
>>>>  		struct consumer_output *consumer)
>>>>  {
>>>> -	int ret;
>>>> -	char *metadata_str = NULL;
>>>> -	size_t len, offset;
>>>> +	int ret_val;
>>>> +	ssize_t ret;
>>>>  	struct consumer_socket *socket;
>>>>  
>>>>  	assert(registry);
>>>> @@ -391,7 +447,7 @@ static int push_metadata(struct ust_registry_session *registry,
>>>>  	 * no start has been done previously.
>>>>  	 */
>>>>  	if (!registry->metadata_key) {
>>>> -		ret = 0;
>>>> +		ret_val = 0;
>>>>  		goto error_rcu_unlock;
>>>>  	}
>>>>  
>>>> @@ -399,7 +455,7 @@ static int push_metadata(struct ust_registry_session *registry,
>>>>  	socket = consumer_find_socket_by_bitness(registry->bits_per_long,
>>>>  			consumer);
>>>>  	if (!socket) {
>>>> -		ret = -1;
>>>> +		ret_val = -1;
>>>>  		goto error_rcu_unlock;
>>>>  	}
>>>>  
>>>> @@ -414,54 +470,19 @@ static int push_metadata(struct ust_registry_session *registry,
>>>>  	 * ability to reorder the metadata it receives.
>>>>  	 */
>>>>  	pthread_mutex_lock(socket->lock);
>>>> -	pthread_mutex_lock(&registry->lock);
> 
> another question: We don't seem to need nesting of the registry lock
> inside the socket lock anymore on the sessiond side. We probably have
> some comments to update around those locks elsewhere ?

Well not sure here. This function has been split in two where this one
finds the socket and lock it then calls ust_app_push_metadata that
*locks* the registry. So there is still nesting needed.
ust_app_push_metadata() *MUST* be called with the socket lock (it is
documented). So to that extent, the registry lock still nest in the
socket lock.

> 
> [...]
>>>> +/*
>>>> + * Commands sent from the consumerd to the sessiond to request
>>>> + * if new metadata is available
>>>> + */
>>>> +struct lttcomm_metadata_request_msg {
>>>> +	unsigned int session_id; /* Tracing session id */
>>>> +	uint32_t bits_per_long; /* Consumer ABI */
>>>> +	uint32_t uid;
>>>> +	uint64_t key; /* Metadata channel key. */
>>>> +} LTTNG_PACKED;
>>>
>>> What happens for per-pid UST buffers ? This message seems to be quite
>>> specific to per-uid buffers ?
>>
>> We ignore every value and use the session id only for per PID. The
>> consumer has no idea if the metadata channel is per UID or PID on the
>> sessiond side so we lookup per uid using all of the values, if failure,
>> per pid with only the session id.
> 
> OK, this should be clearly documented in the code.

I'll make sure of that.

David

> 
> Thanks,
> 
> Mathieu
>