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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200628124311.047fb1a2@oasis.local.home>
Date:   Sun, 28 Jun 2020 12:43:11 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Yordan Karadzhov <y.karadz@...il.com>,
        Tzvetomir Stoyanov <tz.stoyanov@...il.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Jason Behmer <jbehmer@...gle.com>,
        Julia Lawall <julia.lawall@...ia.fr>,
        Clark Williams <williams@...hat.com>,
        bristot <bristot@...hat.com>, Daniel Wagner <wagi@...om.org>,
        Darren Hart <dvhart@...are.com>,
        Jonathan Corbet <corbet@....net>,
        "Suresh E. Warrier" <warrier@...ux.vnet.ibm.com>
Subject: Re: [PATCH 1/3] ring-buffer: Have nested events still record
 running time stamp

On Mon, 29 Jun 2020 01:23:23 +0900
Masami Hiramatsu <mhiramat@...nel.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