From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Subject: Re: [RFC PATCH v1 07/25] printk-rb: add functionality required by printk Date: Mon, 18 Feb 2019 16:59:21 +0100 Message-ID: <20190218155921.rxksi2hyasxnmu7a@pathway.suse.cz> References: <20190212143003.48446-1-john.ogness@linutronix.de> <20190212143003.48446-8-john.ogness@linutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20190212143003.48446-8-john.ogness@linutronix.de> Sender: linux-kernel-owner@vger.kernel.org To: John Ogness Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Sergey Senozhatsky , Steven Rostedt , Daniel Wang , Andrew Morton , Linus Torvalds , Greg Kroah-Hartman , Alan Cox , Jiri Slaby , Peter Feiner , linux-serial@vger.kernel.org, Sergey Senozhatsky List-Id: linux-serial@vger.kernel.org On Tue 2019-02-12 15:29:45, John Ogness wrote: > The printk subsystem needs to be able to query the size of the ring > buffer, seek to specific entries within the ring buffer, and track > if records could not be stored in the ring buffer. > > diff --git a/lib/printk_ringbuffer.c b/lib/printk_ringbuffer.c > index c2ddf4cb9f92..ce33b5add5a1 100644 > --- a/lib/printk_ringbuffer.c > +++ b/lib/printk_ringbuffer.c > @@ -175,11 +175,16 @@ void prb_commit(struct prb_handle *h) > head = PRB_WRAP_LPOS(rb, head, 1); > continue; > } > + while (atomic_long_read(&rb->lost)) { > + atomic_long_dec(&rb->lost); > + rb->seq++; The lost-messages handling should be in a separate patch. At least I do not see any close relation with prb_iter_seek(). I would personally move prb_iter_seek() to the 5th patch that implements the other get/iterator functions. On the contrary, the patch adding support for lost messages should implement a way how to inform the user about lost messages. E.g. to add a warning when some space becomes available again. > + } > e->seq = ++rb->seq; > head += e->size; > changed = true; > } > atomic_long_set_release(&rb->head, res); > + > atomic_dec(&rb->ctx); > > if (atomic_long_read(&rb->reserve) == res) > @@ -486,3 +491,93 @@ int prb_iter_wait_next(struct prb_iterator *iter, char *buf, int size, u64 *seq) > > return ret; > } > + > +/* > + * prb_iter_seek: Seek forward to a specific record. > + * @iter: Iterator to advance. > + * @seq: Record number to advance to. > + * > + * Advance @iter such that a following call to prb_iter_data() will provide > + * the contents of the specified record. If a record is specified that does > + * not yet exist, advance @iter to the end of the record list. > + * > + * Note that iterators cannot be rewound. So if a record is requested that > + * exists but is previous to @iter in position, @iter is considered invalid. > + * > + * It is safe to call this function from any context and state. > + * > + * Returns 1 on succces, 0 if specified record does not yet exist (@iter is > + * now at the end of the list), or -EINVAL if @iter is now invalid. > + */ Do we really need to distinguish when the iterator is invalid and when we are at the end of the buffer? It seems that the reaction in both situation always is to call prb_iter_init(&iter, &printk_rb, &some_seq). I am still a bit confused what your prb_iter_init() does. Therefore I am not sure what it is supposed to do. Anyway, it seems to be typically used when you need to start from tail. I would personally do something like (based on my code in reply to 5th patch: int prb_iter_seek_to_seq(struct prb_iterator *iter, u64 seq) { int ret; ret = prb_iter_tail_entry(iter); while (!ret && iter->entry->seq != seq) ret = prb_iter_next_valid_entry(iter); return ret; } When I see it, I would make the functionality more obvious by renaming: prb_iter_tail_entry() -> prb_iter_set_tail_entry() > +int prb_iter_seek(struct prb_iterator *iter, u64 seq) > +{ > + u64 cur_seq; > + int ret; > + > + /* first check if the iterator is already at the wanted seq */ > + if (seq == 0) { > + if (iter->lpos == PRB_INIT) > + return 1; > + else > + return -EINVAL; > + } > + if (iter->lpos != PRB_INIT) { > + if (prb_iter_data(iter, NULL, 0, &cur_seq) >= 0) { > + if (cur_seq == seq) > + return 1; > + if (cur_seq > seq) > + return -EINVAL; > + } > + } > + > + /* iterate to find the wanted seq */ > + for (;;) { > + ret = prb_iter_next(iter, NULL, 0, &cur_seq); > + if (ret <= 0) > + break; > + > + if (cur_seq == seq) > + break; > + > + if (cur_seq > seq) { > + ret = -EINVAL; > + break; > + } This is a good example why prb_iter_data() and prb_iter_next() is a weird interface. You need to read the documentation very carefully to understand the difference (functionality, error codes). At least for me, it is far from obvious why they are used this way and if it is correct. Best Regards, Petr > + } > + > + return ret; > +}