Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 25 Jun 2019 17:55:48 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Petr Mladek <pmladek@suse.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrea Parri <andrea.parri@amarulasolutions.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer
 implementation
Message-ID: <20190625085548.GA532@jagdpanzerIV>
References: <20190607162349.18199-1-john.ogness@linutronix.de>
 <20190607162349.18199-2-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190607162349.18199-2-john.ogness@linutronix.de>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On (06/07/19 18:29), John Ogness wrote:
[..]
> +static void add_descr_list(struct prb_reserved_entry *e)
> +{
> +	struct printk_ringbuffer *rb = e->rb;
> +	struct prb_list *l = &rb->descr_list;
> +	struct prb_descr *d = e->descr;
> +	struct prb_descr *newest_d;
> +	unsigned long newest_id;
> +
> +	/* set as newest */
> +	do {
> +		/* MB5: synchronize add descr */
> +		newest_id = smp_load_acquire(&l->newest);
> +		newest_d = TO_DESCR(rb, newest_id);
> +
> +		if (newest_id == EOL)
> +			WRITE_ONCE(d->seq, 1);
> +		else
> +			WRITE_ONCE(d->seq, READ_ONCE(newest_d->seq) + 1);
> +		/*
> +		 * MB5: synchronize add descr
> +		 *
> +		 * In particular: next written before cmpxchg
> +		 */
> +	} while (cmpxchg_release(&l->newest, newest_id, e->id) != newest_id);
> +
> +	if (unlikely(newest_id == EOL)) {
> +		/* no previous newest means we *are* the list, set oldest */
> +
> +		/*
> +		 * MB UNPAIRED
> +		 *
> +		 * In particular: Force cmpxchg _after_ cmpxchg on newest.
> +		 */
> +		WARN_ON_ONCE(cmpxchg_release(&l->oldest, EOL, e->id) != EOL);
> +	} else {
> +		/* link to previous chain */
> +
> +		/*
> +		 * MB6: synchronize link descr
> +		 *
> +		 * In particular: Force cmpxchg _after_ cmpxchg on newest.
> +		 */
> +		WARN_ON_ONCE(cmpxchg_release(&newest_d->next,
> +					     EOL, e->id) != EOL);
> +	}
> +}

[..]

> +char *prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
> +		  unsigned int size)
> +{
> +	struct prb_datablock *b;
> +	struct prb_descr *d;
> +	char *buf;
> +
> +	if (size == 0)
> +		return NULL;
> +
> +	size += sizeof(struct prb_datablock);
> +	size = DATA_ALIGN_SIZE(size);
> +	if (size > DATAARRAY_SIZE(rb))
> +		return NULL;
> +
> +	e->rb = rb;
> +
> +	local_irq_save(e->irqflags);
> +
> +	if (!assign_descr(e))
> +		goto err_out;
> +
> +	d = e->descr;
> +	WRITE_ONCE(d->id, e->id);
> +
> +	if (!data_reserve(e, size)) {
> +		/* put invalid descriptor on list, can still be traversed */
> +		WRITE_ONCE(d->next, EOL);
> +		add_descr_list(e);
> +		goto err_out;
> +	}

I'm wondering if prb can always report about its problems. Including the
cases when things "go rather bad".

Suppose we have

	printk()
	 prb_reserve()
	  !data_reserve()
	    add_descr_list()
	     WARN_ON_ONCE()
	      printk()
	       prb_reserve()
	        !assign_descr(e)   << lost WARN_ON's "printk" or "printks"?

In general, assuming that there might be more error printk-s either
called directly directly from prb->printk on indirectly, from
prb->ABC->printk.

Also note,
Lost printk-s are not going to be accounted as 'lost' automatically.
It seems that for printk() there is no way to find out that it has
recursed from printk->prb_commit but hasn't succeeded in storing
recursive messages. I'd say that prb_reserve() err_out should probably
&rb->lost++.

	-ss