Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

[Brendan Higgins <brendanhiggins@google.com>]

On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-07-22 15:30:49)
> > > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > >
> > > > What's the calling context of the assertions and expectations? I still
> > > > don't like the fact that string stream needs to allocate buffers and
> > > > throw them into a list somewhere because the calling context matters
> > > > there.
> > >
> > > The calling context is the same as before, which is anywhere.
> >
> > Ok. That's concerning then.
> >
> > >
> > > > I'd prefer we just wrote directly to the console/log via printk
> > > > instead. That way things are simple because we use the existing
> > > > buffering path of printk, but maybe there's some benefit to the string
> > > > stream that I don't see? Right now it looks like it builds a string and
> > > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > > just writing directly with printk.
> > >
> > > It's just buffering it so the whole string gets printed uninterrupted.
> > > If we were to print out piecemeal to printk, couldn't we have another
> > > call to printk come in causing it to garble the KUnit message we are
> > > in the middle of printing?
> >
> > Yes, printing piecemeal by calling printk many times could lead to
> > interleaving of messages if something else comes in such as an interrupt
> > printing something. Printk has some support to hold "records" but I'm
> > not sure how that would work here because KERN_CONT talks about only
> > being used early on in boot code. I haven't looked at printk in detail
> > though so maybe I'm all wrong and KERN_CONT just works?
>
> KERN_CONT does not guarantee that the message will get printed
> together. The pieces get interleaved with messages printed in
> parallel.
>
> Note that KERN_CONT was originally really meant to be used only during
> boot. It was later used more widely and ended in the best effort category.
>
> There were several attempts to make it more reliable. But it was
> always either too complicated or error prone or both.
>
> You need to use your own buffering if you rely want perfect output.
> The question is if it is really worth the complexity. Also note that
> any buffering reduces the chance that the messages will reach
> the console.

Seems like that settles it then. Thanks!

> BTW: There is a work in progress on a lockless printk ring buffer.
> It will make printk() more secure regarding deadlocks. But it might
> make transparent handling of continuous lines even more tricky.
>
> I guess that local buffering, before calling printk(), will be
> even more important then. Well, it might really force us to create
> an API for it.

Cool! Can you CC me on that discussion?

> > Can printk be called once with whatever is in the struct? Otherwise if
> > this is about making printk into a structured log then maybe printk
> > isn't the proper solution anyway. Maybe a dev interface should be used
> > instead that can handle starting and stopping tests (via ioctl) in
> > addition to reading test results, records, etc. with read() and a
> > clearing of the records. Then the seqfile API works naturally. All of
> > this is a bit premature, but it looks like you're going down the path of
> > making something akin to ftrace that stores binary formatted
> > assertion/expectation records in a lockless ring buffer that then
> > formats those records when the user asks for them.
>
> IMHO, ftrace postpones the text formatting primary because it does not
> not want to slow down the traced code more than necessary. It is yet
> another layer and there should be some strong reason for it.

Noted. Yeah, I would prefer avoiding printing out the info at a separate time.

> > I can imagine someone wanting to write unit tests that check conditions
> > from a simulated hardirq context via irq works (a driver mock
> > framework?), so this doesn't seem far off.
>
> Note that stroring the messages into the printk log is basically safe in any
> context. It uses temporary per-CPU buffers for recursive messages and
> in NMI. The only problem is panic() when some CPU gets stuck with the
> lock taken. This will get solved by the lockless ringbuffer. Also
> the temporary buffers will not be necessary any longer.

Sure, I think Stephen's concern is all the supporting code that is
involved. Not printk specifically. It just means a lot more of KUnit
has to be IRQ safe.

> Much bigger problems are with consoles. There are many of them. It
> means a lot of code and more locks involved, including scheduler
> locks. Note that console lock is a semaphore.

That shouldn't affect us though, right? As long as we continue to use
the printk interface?