Hi Petr, Nit: For the patch subject, remove the word "a":
"Create a helper function to decide whether more space is needed" More below... On 2025-11-07, Petr Mladek <[email protected]> wrote: > The decision whether some more space is needed is tricky in the printk > ring buffer code: > > 1. The given lpos values might overflow. A subtraction must be used > instead of a simple "lower than" check. > > 2. Another CPU might reuse the space in the mean time. It can be > detected when the subtraction is bigger than DATA_SIZE(data_ring). > > 3. There is exactly enough space when the result of the subtraction > is zero. But more space is needed when the result is exactly > DATA_SIZE(data_ring). > > Add a helper function to make sure that the check is done correctly > in all situations. Also it helps to make the code consistent and > better documented. > > Suggested-by: John Ogness <[email protected]> > Link: https://lore.kernel.org/r/[email protected] > Signed-off-by: Petr Mladek <[email protected]> > --- > kernel/printk/printk_ringbuffer.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/kernel/printk/printk_ringbuffer.c > b/kernel/printk/printk_ringbuffer.c > index 3e6fd8d6fa9f..ede3039dd041 100644 > --- a/kernel/printk/printk_ringbuffer.c > +++ b/kernel/printk/printk_ringbuffer.c > @@ -411,6 +411,23 @@ static bool data_check_size(struct prb_data_ring > *data_ring, unsigned int size) > return to_blk_size(size) <= DATA_SIZE(data_ring) / 2; > } > > +/* > + * Compare the current and requested logical position and decide > + * whether more space needed. > + * > + * Return false when @lpos_current is already at or beyond @lpos_target. > + * > + * Also return false when the difference between the positions is bigger > + * than the size of the data buffer. It might happen only when the caller > + * raced with another CPU(s) which already made and used the space. > + */ > +static bool need_more_space(struct prb_data_ring *data_ring, > + unsigned long lpos_current, > + unsigned long lpos_target) > +{ > + return lpos_target - lpos_current - 1 < DATA_SIZE(data_ring); > +} > + > /* Query the state of a descriptor. */ > static enum desc_state get_desc_state(unsigned long id, > unsigned long state_val) > @@ -577,7 +594,7 @@ static bool data_make_reusable(struct printk_ringbuffer > *rb, > unsigned long id; > > /* Loop until @lpos_begin has advanced to or beyond @lpos_end. */ > - while ((lpos_end - lpos_begin) - 1 < DATA_SIZE(data_ring)) { > + while (need_more_space(data_ring, lpos_begin, lpos_end)) { > blk = to_block(data_ring, lpos_begin); > > /* > @@ -668,7 +685,7 @@ static bool data_push_tail(struct printk_ringbuffer *rb, > unsigned long lpos) > * sees the new tail lpos, any descriptor states that transitioned to > * the reusable state must already be visible. > */ > - while ((lpos - tail_lpos) - 1 < DATA_SIZE(data_ring)) { > + while (need_more_space(data_ring, tail_lpos, lpos)) { > /* > * Make all descriptors reusable that are associated with > * data blocks before @lpos. > @@ -1148,8 +1165,14 @@ static char *data_realloc(struct printk_ringbuffer > *rb, unsigned int size, > > next_lpos = get_next_lpos(data_ring, blk_lpos->begin, size); > > - /* If the data block does not increase, there is nothing to do. */ > - if (head_lpos - next_lpos < DATA_SIZE(data_ring)) { > + /* > + * Use the current data block when the size does not increase. I would like to expand the above sentence so that it is a bit clearer how it relates to the new check. Perhaps: * Use the current data block when the size does not increase, i.e. * when @head_lpos is already able to accommodate the new @next_lpos. > + * > + * Note that need_more_space() could never return false here because > + * the difference between the positions was bigger than the data > + * buffer size. The data block is reopened and can't get reused. > + */ > + if (!need_more_space(data_ring, head_lpos, next_lpos)) { > if (wrapped) > blk = to_block(data_ring, 0); > else > -- > 2.51.1 Otherwise, LGTM. Thanks for choosing a name that presents contextual purpose rather than simply function. Reviewed-by: John Ogness <[email protected]> _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
