Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer

[John Ogness]

On 2020-02-21, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> new file mode 100644
>> index 000000000000..796257f226ee
>> --- /dev/null
>> +++ b/kernel/printk/printk_ringbuffer.c
>> +static struct prb_data_block *to_block(struct prb_data_ring *data_ring,
>> +				       unsigned long begin_lpos)
>> +{
>> +	char *data = &data_ring->data[DATA_INDEX(data_ring, begin_lpos)];
>> +
>> +	return (struct prb_data_block *)data;
>
> Nit: Please, use "blk" instead of "data". I was slightly confused
> because "data" is also one member of struct prb_data_block.

OK.

>> +/* The possible responses of a descriptor state-query. */
>> +enum desc_state {
>> +	desc_miss,	/* ID mismatch */
>> +	desc_reserved,	/* reserved, but still in use by writer */
>> +	desc_committed, /* committed, writer is done */
>> +	desc_reusable,	/* free, not used by any writer */
>
> s/not used/not yet used/

OK.

>> +EXPORT_SYMBOL(prb_reserve);
>
> Please, do not export symbols if there are no plans to actually
> use them from modules. It will be easier to rework the code
> in the future. Nobody would need to worry about external
> users.
>
> Please, do so everywhere in the patchset.

You are correct.

The reason I exported them is that I could run my test module. But since
the test module will not be part of the kernel source, I'll just hack
the exports in when doing my testing.

>> +static char *get_data(struct prb_data_ring *data_ring,
>> +		      struct prb_data_blk_lpos *blk_lpos,
>> +		      unsigned long *data_size)
>> +{
>> +	struct prb_data_block *db;
>> +
>> +	/* Data-less data block description. */
>> +	if (blk_lpos->begin == INVALID_LPOS &&
>> +	    blk_lpos->next == INVALID_LPOS) {
>> +		return NULL;
>
> Nit: There is no need for "else" after return. checkpatch.pl usually
> complains about it ;-)

OK.

>> +/*
>> + * Read the record @id and verify that it is committed and has the sequence
>> + * number @seq. On success, 0 is returned.
>> + *
>> + * Error return values:
>> + * -EINVAL: A committed record @seq does not exist.
>> + * -ENOENT: The record @seq exists, but its data is not available. This is a
>> + *          valid record, so readers should continue with the next seq.
>> + */
>> +static int desc_read_committed(struct prb_desc_ring *desc_ring,
>> +			       unsigned long id, u64 seq,
>> +			       struct prb_desc *desc)
>> +{
>
> I was few times confused whether this function reads the descriptor
> a safe way or not.
>
> Please, rename it to make it clear that does only a check.
> For example, check_state_commited().

This function _does_ read. It is a helper function of prb_read() to
_read_ the descriptor. It is an extended version of desc_read() that
also performs various checks that the descriptor is committed.

I will update the function description to be more similar to desc_read()
so that it is obvious that it is "getting a copy of a specified
descriptor".

John Ogness