Re: [PATCH v9 11/20] gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Sep 25, 2020 at 12:57:59PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 24, 2020 at 3:46 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > On Thu, Sep 24, 2020 at 03:32:48PM +0800, Kent Gibson wrote:
> > > On Wed, Sep 23, 2020 at 07:18:08PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Sep 22, 2020 at 5:36 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > >
> > > > > Add support for the GPIO_V2_LINE_SET_VALUES_IOCTL.
> > > >
> > > > > +static long linereq_set_values_unlocked(struct linereq *lr,
> > > > > +                                       struct gpio_v2_line_values *lv)
> > > > > +{
> > > > > +       DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
> > > > > +       struct gpio_desc **descs;
> > > > > +       unsigned int i, didx, num_set;
> > > > > +       int ret;
> > > > > +
> > > > > +       bitmap_zero(vals, GPIO_V2_LINES_MAX);
> > > > > +       for (num_set = 0, i = 0; i < lr->num_lines; i++) {
> > > > > +               if (lv->mask & BIT_ULL(i)) {
> > > >
> > > > Similar idea
> > > >
> > > > DECLARE_BITMAP(mask, 64) = BITMAP_FROM_U64(lv->mask);
> > > >
> > > > num_set = bitmap_weight();
> > > >
> > >
> > > I had played with this option, but bitmap_weight() counts all
> > > the bits set in the mask - which considers bits >= lr->num_lines.
> > > So you would need to mask lv->mask before converting it to a bitmap.
> > > (I'm ok with ignoring those bits in case userspace wants to be lazy and
> > > use an all 1s mask.)
> > >
> > > But since we're looping over the bitmap anyway we may as well just
> > > count as we go.
> > >
> > > > for_each_set_bit(i, mask, lr->num_lines)
> > > >
> > >
> > > Yeah, that should work.  I vaguely recall trying this and finding it
> > > generated larger object code, but I'll give it another try and if it
> > > works out then include it in v10.
> > >
> >
> > Tried it again and, while it works, it does increase the size of
> > gpiolib-cdev.o as follows:
> >
> >           u64   ->   bitmap
> > x86_64   28360       28616
> > i386     22056       22100
> > aarch64  37392       37600
> > mips32   28008       28016
> 
> Yes, that's pity... See below.
> 
> > So for 64-bit platforms changing to bitmap generates larger code,
> > probably as we are forcing them to use 32-bit array semantics where
> > before they could use the native u64.  For 32-bit there is a much
> > smaller difference as they were already using 32-bit array semantics
> > to realise the u64.
> >
> > Those are for some of my test builds, so obviously YMMV.
> >
> > It is also only for changing linereq_get_values(), which has three
> > instances of the loop.  linereq_set_values_unlocked() has another two,
> > so you could expect another increase of ~2/3 of that seen here if we
> > change that as well.
> >
> > The sizeable increase in x86_64 was what made me revert this last time,
> > and I'm still satisfied with that choice.  Are you still eager to switch
> > to for_each_set_bit()?
> 
> I already asked once about short cut for for_each_set_bit in case of
> constant nbits parameter when it's <= BITS_PER_LONG, but here it seems
> we have variadic amount of lines, dunno if compiler can prove that
> it's smaller than long. In any case my point is that code readability
> has a preference vs. memory footprint (except hot paths) and if we are
> going to fix this it should be done in general. That said, if
> maintainers are okay with that I would prefer bitmap API over
> open-coded pieces.
> 

Agreed - if the bitmap ops made better use of the architecure then I'd
change to bitmap without question - it is more readable.

Bart and Linus - do you have any preference?

> Also note, that it will be easier to extend in the future if needed
> (if we want to have more than BITS_PER_LONG [64] lines to handle).
> 

Yeah, I think we can rule that one out - while I had initially written
the uAPI with the option of > 64 lines we decided on the hard limit to
keep things simple.  And any user that needs to request more than 64
lines in one request shouldn't be using the GPIO uAPI.

Cheers,
Kent.