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

[Mathieu Desnoyers]

* David Goulet (dgoulet at efficios.com) wrote:
> 
> 
> 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.

Why do we need to lock the socket lock insite push_metadata() ?

We could probably take both locks one after the other (not nested)
within ust_app_push_metadata(), given that we now allow out-of-order
metadata segments ?

Thanks,

Mathieu

> 
> > 
> > [...]
> >>>> +/*
> >>>> + * 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
> > 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com