Re: numlist API Re: [RFC PATCH v4 1/9] printk-rb: add a new printk ringbuffer implementation

[Petr Mladek <pmladek@suse.com>]

On Wed 2019-08-28 16:03:38, John Ogness wrote:
> On 2019-08-28, Petr Mladek <pmladek@suse.com> wrote:
> > I only think that, especially, numlist API is too generic in v4.
> > It is not selfcontained. The consistency depends on external barriers.
> >
> > I believe that it might become fully self-contained and consistent
> > if we reduce possibilities of the generic usage. In particular,
> > the numlist should allow only linking of reusable structures
> > stored in an array.
> 
> OK. I will make the numlist the master of the ID-to-node mapping. To
> implement the getdesc() callback of the dataring, the printk_ringbuffer
> can call a numlist mapping function. Also, numlist will need to provide
> a function to bump the descriptor version (as your previous idea already
> showed).

Sounds good.

> I plan to change the array to be numlist nodes. The ID would move into
> the numlist node structure and a void-pointer private would be added so
> that the numlist user can add private data (for printk_ringbuffer that
> would just be a pointer to the dataring structure). When the
> printk_ringbuffer gets a never-used numlist node, it can set the private
> field.

I am not sure that I get the full picture. It would help to see some
snippet of the code (struct declaration).

Anyway, adding void-pointer into struct numlist looks like classic
(userspace?) implementation of dynamically linked structures.

I do not have strong opinion. But I would prefer to stay with
the kernel-style. I mean that the numlist structure is part
of the linked structures. And container_of() is eventually
used to get pointer to the upper structure. Also passing values
via a pointer with generic name (data) slightly complicates the code.

I know that numlist is special because id is used to get the pointer
of the numlist and also the upper structure. Anyway, the kernel
style looks more familiar to me in the kernel context.
But I'll leave it up to you.


> This has the added benefit of making it easy to detect accidental
> never-used descriptor usage when reading dataring garbage. This was
> non-trivial and I'm still not sure I solved it correctly. (I've already
> spent a week working on a definitive answer to your email[0] asking
> about this.)

Would a check for NULL data pointer help here? Well, it might be
motivation for using the pointer.

I wonder if begin_lpos == end_lpos check might be used to detect
never used descriptors. It is already used in another situation
in dataring_desc_init(). IMHO, the values even do not need to be
unaligned. But I might miss something.

Best Regards,
Petr