linux-kernel - Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Jun 2019 00:30:26 +0200
From:   John Ogness <john.ogness@...utronix.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation

On 2019-06-18, Peter Zijlstra <peterz@...radead.org> wrote:
>> +/**
>> + * DOC: memory barriers
>
> What's up with that 'DOC' crap?

The separate documentation in
Documentation/core-api/printk-ringbuffer.rst references this so it
automatically shows up in the kernel docs. An external reference
requires the DOC keyword.

Maybe the memory barrier descriptions do not belong in the kernel docs?

>> + *
>> + * Writers
>> + * -------
>> + * The main issue with writers is expiring/invalidating old data blocks in
>> + * order to create new data blocks. This is performed in 6 steps that must
>> + * be observed in order by all writers to allow cooperation. Here is a list
>> + * of the 6 steps and the named acquire/release memory barrier pairs that
>> + * are used to synchronized them:
>> + *
>> + * * old data invalidation (MB1): Pushing rb.data_list.oldest forward.
>> + *   Necessary for identifying if data has been expired.
>> + *
>> + * * new data reservation (MB2): Pushing rb.data_list.newest forward.
>> + *   Necessary for validating data.
>> + *
>> + * * assign the data block to a descriptor (MB3): Setting data block id to
>> + *   descriptor id. Necessary for finding the descriptor associated with th
>> + *   data block.
>> + *
>> + * * commit data (MB4): Setting data block data_next. (Now data block is
>> + *   valid). Necessary for validating data.
>> + *
>> + * * make descriptor newest (MB5): Setting rb.descr_list.newest to descriptor.
>> + *   (Now following new descriptors will be linked to this one.) Necessary for
>> + *   ensuring the descriptor's next is set to EOL before adding to the list.
>> + *
>> + * * link descriprtor to previous newest (MB6): Setting the next of the
>> + *   previous descriptor to this one. Necessary for correctly identifying if
>> + *   a descriptor is the only descriptor on the list.
>> + *
>> + * Readers
>> + * -------
>> + * Readers only make of smb_rmb() to ensure that certain critical load
>> + * operations are performed in an order that allows readers to evaluate if
>> + * the data they read is really valid.
>> + */
>
> This isn't really helping much I feel. It doesn't begin to describe the
> ordering. But maybe the code makes more sense.

Sorry. I really have no feel about what (or how) exactly I should
document the memory barriers. I think the above comments make sense when
someone understands the details of the implementation. But perhaps it
should describe things such that someone without knowledge of the
implementation would understand what the memory barriers are for? That
would significantly increase the amount of text as I would have to
basically explain the implementation.

I would appreciate it if you could point out a source file that
documents its memory barriers the way you would like to see these memory
barriers documented.

John Ogness