Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Oct 13, 2020 at 10:53 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> 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?
>

Yeah I wanted to keep it. Why? We allow plugging into external event
loops by providing a helper for accessing the underlying file
descriptor but I think we still should have some basic wrappers for
poll(). What exactly are you referring to as "streaming interface"?

> > > 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.
>

Precisely that.

Bartosz