Re: [PATCH 1/3] ring-buffer: Have nested events still record running time stamp

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 29 Jun 2020 01:23:23 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > Here's the design of this solution:
> > 
> >  All this is per cpu, and only needs to worry about nested events (not
> >  parallel events).
> >  
> 
> This looks basically good to me, but I have some comments below.
> (just a clean up)

Thanks Masami!

>  
> > The players:
> > 
> >  write_tail: The index in the buffer where new events can be written to.
> >      It is incremented via local_add() to reserve space for a new event.
> > 
> >  before_stamp: A time stamp set by all events before reserving space.
> > 
> >  write_stamp: A time stamp updated by events after it has successfully
> >      reserved space.  
> 
> So these stamps works like a seq-lock to detect interruption (from both
> interrupted process and interrpting process)

Yes.

> 
> > 
> >  next_write: A copy of "write_tail" used to help with races.  
> 
> It seems this player does nothing.

Bah, you're right. With removing the one path that Mathieu suggested,
took this player out of the game. Will remove in v2. Thanks for
pointing this out.

> 
> > 
> > 	/* Save the current position of write */
> >  [A]	w = local_read(write_tail);
> > 	barrier();
> > 	/* Read both before and write stamps before touching anything */
> > 	before = READ_ONCE(before_stamp);
> > 	after = local_read(write_stamp);  
> 
> Are there any reason to use READ_ONCE() and local_read()?
> (In the code, both are local64_read())

Thanks for pointing this out. before_stamp was originally just a normal
variable, but in discussions with Mathieu, it became apparent that it
needed to be atomic as well.

I'll update the change log in v2.


> 
> > 	barrier();
> > 
> > 	/*
> > 	 * If before and after are the same, then this event is not
> > 	 * interrupting a time update. If it is, then reserve space for adding
> > 	 * a full time stamp (this can turn into a time extend which is
> > 	 * just an extended time delta but fill up the extra space).
> > 	 */
> > 	if (after != before)
> > 		abs = true;
> > 
> > 	ts = clock();
> > 
> > 	/* Now update the before_stamp (everyone does this!) */
> >  [B]	WRITE_ONCE(before_stamp, ts);
> > 
> > 	/* Read the current next_write and update it to what we want write
> > 	 * to be after we reserve space. */
> >  	next = READ_ONCE(next_write);
> > 	WRITE_ONCE(next_write, w + len);  
> 
> This seems meaningless, because neither "next" nor "next_write"
> are not refered.

and they are now meaningless, but wasn't in the RFC. I'll remove it.

> 
> > 
> > 	/* Now reserve space on the buffer */
> >  [C]	write = local_add_return(len, write_tail);
> > 
> > 	/* Set tail to be were this event's data is */
> > 	tail = write - len;

[...]

> >  
> > -	/* Don't let the compiler play games with cpu_buffer->tail_page */
> > -	tail_page = info->tail_page = READ_ONCE(cpu_buffer->tail_page);
> > -	write = local_add_return(info->length, &tail_page->write);
> > +	next = READ_ONCE(cpu_buffer->next_write);
> > +	WRITE_ONCE(cpu_buffer->next_write, w + info->length);  
> 
> So, this next may have no effect.

And I'll remove them.

Thanks for reviewing!

-- Steve