On Wed 2020-09-02 13:26:14, John Ogness wrote:
> On 2020-09-02, Petr Mladek <pmladek@xxxxxxxx> wrote:
> >> +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) ||
> >
> > First, define 5 desc_states, something like:
> >
> > enum desc_state {
> > desc_miss = -1, /* ID mismatch */
> > desc_modified = 0x0, /* reserved, being modified by writer */
>
> I prefer the "desc_reserved" name. It may or may not have be modified yet.
Yeah, "desc_reserved" sounds better. I probably just wanted to free my
fantasy from the current code ;-)
> > 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))
>
> This makes sense and will get us back the bit we lost because of
> finalization.
Yup. Which is good especially on 32-bit architectures.
> I am wondering if VMCOREINFO should include a DESC_FLAGS_MASK so that
> crash tools could at least successfully iterate the ID's, even if they
> didn't know what all the flag values mean (in the case that more bits
> are added later).
Good point. I am just not sure whether they should try read all ids
or they should refuse reading anything when a new bit is added.
Well, I really hope that we will not need new states anytime soon.
It would need a really strong reason.
I personally can't think about any use case. pr_cont() was special
because it was the writer side. All other steps of the printk rework
are on the reader side.
I believe that we are getting close with all the ring buffer code.
And I have good feeling about it.
Best Regards,
Petr
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
