Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Oct 13, 2020 at 09:45:04AM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 13, 2020 at 2:53 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 12, 2020 at 05:15:25PM +0200, Bartosz Golaszewski wrote:
> > > Hi!
> > >
> > > One of the things I'd like to address in libgpiod v2.0 is excessive
> > > stack usage with struct gpiod_line_bulk. This structure is pretty big
> > > right now: it's an array 64 pointers + 4 bytes size. That amounts to
> > > 260 bytes on 32-bit and 516 bytes on 64-bit architectures
> > > respectively. It's also used everywhere as all functions dealing with
> > > single lines eventually end up calling bulk counterparts.
> > >
> > > I have some ideas for making this structure smaller and I thought I'd
> > > run them by you.
> > >
> > > The most obvious approach would be to make struct gpiod_line_bulk
> > > opaque and dynamically allocated. I don't like this idea due to the
> > > amount of error checking this would involve and also calling malloc()
> > > on virtually every value read, event poll etc.
> > >
> > > Another idea is to use embedded list node structs (see include/list.h
> > > in the kernel) in struct gpiod_line and chain the lines together with
> > > struct gpiod_line_bulk containing the list head. That would mean only
> > > being able to store each line in a single bulk object. This is
> > > obviously too limiting.
> > >
> >
> > I don't think I've ever gotten my head fully around the libgpiod API,
> > or all its use cases, and I'm not clear on why this is too limiting.
> >
> 
> For instance: we pass one bulk object to gpiod_line_event_wait_bulk()
> containing the lines to poll and use another to store the lines for
> which events were detected. Lines would need to live in two bulks.
> 

Ahh, ok.  So you want to keep that?  I prefer a streaming interface, but
I guess some prefer the select/poll style?

> > What is the purpose of the gpiod_line_bulk, and how does that differ from the
> > gpio_v2_line_request?
> >
> 
> struct gpiod_line_bulk simply aggregates lines so that we can easily
> operate on multiple lines at once. Just a convenience helper
> basically.
> 
> > > An idea I think it relatively straightforward without completely
> > > changing the current interface is making struct gpiod_line_bulk look
> > > something like this:
> > >
> > > struct gpiod_line_bulk {
> > >     unsigned int num_lines;
> > >     uint64_t lines;
> > > };
> > >
> > > Where lines would be a bitmap with set bits corresponding to offsets
> > > of lines that are part of this bulk. We'd then provide a function that
> > > would allow the user to get the line without it being updated (so
> > > there's no ioctl() call that could fail). The only limit that we'd
> > > need to introduce here is making it impossible to store lines from
> > > different chips in a single line bulk object. This doesn't make sense
> > > anyway so I'm fine with this.
> > >
> > > What do you think? Do you have any other ideas?
> > >
> >
> > Doesn't that place a strict range limit on offset values, 0-63?
> > The uAPI limits the number of offsets requested to 64, not their value.
> > Otherwise I'd've used a bitmap there as well.
> >
> > Or is there some other mapping happening in the background that I'm
> > missing?
> >
> 
> Nah, you're right of course. The structure should actually look more like:
> 
> struct gpiod_line_bulk {
>     struct gpiod_chip *owner;
>     unsigned int num_lines;
>     uint64_t lines;
> };
> 
> And the 'lines' bitmap should actually refer to offsets at which the
> owning chip stores the line pointers in its own 'lines' array - up to
> 64 lines.
> 
> But we'd still have to sanitize the values when adding lines to a bulk
> object and probably check the return value. I'm wondering if there's a
> better way to store group references to lines on the stack but I'm out
> of ideas.
> 

So you are proposing keeping the bulk of the bulk in the background and
passing around a flyweight in its place.  Makes sense.

Cheers,
Kent.