lkml.org 
[lkml]   [2020]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
Subjectstate names: vas: Re: [PATCH next v3 6/8] printk: ringbuffer: add finalization/extension support
On Mon 2020-08-31 03:16:56, John Ogness wrote:
> Add support for extending the newest data block. For this, introduce
> a new finalization state flag (DESC_FINAL_MASK) that denotes when a
> descriptor may not be extended, i.e. is finalized.

> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -49,14 +49,16 @@
> * Descriptors have three states:
> *
> * reserved
> - * A writer is modifying the record.
> + * A writer is modifying the record. Internally represented as either "0"
> + * or "DESC_COMMIT_MASK".

We should explain the difference between the two values. It might be
enough to add something like:

See "Descriptor Finalization" section for more details."

> @@ -79,6 +81,25 @@
> * committed or reusable queried state. This makes it possible that a valid
> * sequence number of the tail is always available.
> *
> + * Descriptor Finalization
> + * ~~~~~~~~~~~~~~~~~~~~~~~
> + * When a writer calls the commit function prb_commit(), the record may still
> + * continue to be in the reserved queried state. In order for that record to
> + * enter into the committed queried state, that record also must be finalized.
> + * A record can be finalized by three different scenarios:
> + *
> + * 1) A writer can finalize its record immediately by calling
> + * prb_final_commit() instead of prb_commit().
> + *
> + * 2) When a new record is reserved and the previous record has been
> + * committed via prb_commit(), that previous record is finalized.
> + *
> + * 3) When a record is committed via prb_commit() and a newer record
> + * already exists, the record being committed is finalized.
> + *
> + * Until a record is finalized (represented by "DESC_FINAL_MASK"), a writer
> + * may "reopen" that record and extend it with more data.
> + *
> * Data Rings
> * ~~~~~~~~~~
> * The two data rings (text and dictionary) function identically. They exist

[...]

> +/*
> + * Attempt to remove the commit flag so that the record can be modified by a
> + * writer again. This is only possible if the descriptor is not yet finalized.
> + *
> + * Note that on success, the queried state did not change. A non-finalized
> + * record (even with the commit flag set) is in the reserved queried state.
> + */
> +static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
> + u32 caller_id, unsigned long *id_out)
> +{
> + unsigned long prev_state_val;
> + enum desc_state d_state;
> + struct prb_desc desc;
> + struct prb_desc *d;
> + unsigned long id;
> +
> + id = atomic_long_read(&desc_ring->head_id);
> +
> + /*
> + * To minimize unnecessarily reopening a descriptor, first check the
> + * descriptor is in the correct state and has a matching caller ID.
> + */
> + d_state = desc_read(desc_ring, id, &desc);
> + if (d_state != desc_reserved ||
> + !(atomic_long_read(&desc.state_var) & DESC_COMMIT_MASK) ||

This looks like a hack. And similar extra check of the bit is needed
also in desc_read(), see
https://lore.kernel.org/r/878sdvq8kd.fsf@jogness.linutronix.de

I has been actually getting less and less happy with the inconsistency
between names of the bits and states.

Sigh, you will hate me because this would mean a bigger change.
IMHO, it would be much cleaner and help with long-term maintainability
when we do the following two changes:

First, define 5 desc_states, something like:

enum desc_state {
desc_miss = -1, /* ID mismatch */
desc_modified = 0x0, /* reserved, being modified by writer */
desc_committed = 0x1, /* committed by writer, could get reopened */
desc_finalized = 0x2, /* committed, could not longer get modified */
desc_reusable = 0x3, /* free, not yet used by any writer */
};

Second, only 4 variants of the 3 state bits are currently used.
It means that two bits are enough and they might use exactly
the above names:

I mean to do something like:

#define DESC_SV_BITS (sizeof(unsigned long) * 8)
#define DESC_SV(desc_state) ((unsigned long)desc_state << (DESC_SV_BITS - 2))
#define DESC_ST(state_val) ((unsigned long)state_val >> (DESC_SV_BITS - 2))


Then we could have:

static enum desc_state get_desc_state(unsigned long id,
unsigned long state_val)
{
if (id != DESC_ID(state_val))
return desc_miss;

return DESC_ST(state_val);
}

and use in the code:

unsigned long val_committed = id | DESC_SV(desc_committed);


or do

#define DESC_SV(id, desc_state) (id | (unsigned long)desc_state << (DESC_SV_BITS - 2))

and then use DESC_SV(id, DESC_COMMITTED).


I am sorry that I did not came up with this earlier. I know how
painful it is to rework bigger patchsets. But it affects format
of the ring buffer, so we should do it early.

Best Regards,
Petr

PS: I am still middle of review. It looks good so far. I wanted to
send this early and separately because it is a bigger change.

\
 
 \ /
  Last update: 2020-09-02 12:53    [W:0.106 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site