gpiolib gpio_chrdev_release duration is about 30 ms

gpiolib gpio_chrdev_release duration is about 30 ms

[Maxim Kochetkov]

Hi.

I'm using libgpiod in userspace.
I have 6 gpiochip's on my board.
gpiod_line_find takes about 300ms to find GPIO line.

gpiod_line_find calls gpiod_foreach_chip
then gpiod_chip_iter_next
then gpiod_chip_close then close(chip->fd)
then we are going to kernel gpiolib gpio_chrdev_release
then atomic_notifier_chain_unregister
then synchronize_rcu()

synchronize_rcu takes about 30 ms (6*30ms=280ms)

I tried to remove synchronize_rcu from atomic_notifier_chain_unregister 
and gpiod_line_find takes about 2ms now.

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Linus Walleij]

Hi Maxim,

On Fri, Jul 17, 2020 at 2:56 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:

> I'm using libgpiod in userspace.
> I have 6 gpiochip's on my board.
> gpiod_line_find takes about 300ms to find GPIO line.
>
> gpiod_line_find calls gpiod_foreach_chip
> then gpiod_chip_iter_next
> then gpiod_chip_close then close(chip->fd)
> then we are going to kernel gpiolib gpio_chrdev_release
> then atomic_notifier_chain_unregister
> then synchronize_rcu()
>
> synchronize_rcu takes about 30 ms (6*30ms=280ms)
>
> I tried to remove synchronize_rcu from atomic_notifier_chain_unregister
> and gpiod_line_find takes about 2ms now.

Interesting! Can you provide some context? Are you just testing because
curious or do you need to meet a design objective?

Did you use ftrace or similar instrumentation to drill down and find
where time is spent?

Yours,
Linus Walleij

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Maxim Kochetkov]

------------------------------------------
#define GPIOS_NUM 8
static char* gpio_names[GPIOS_NUM] = {"IO0", "IO1", "IO2", "IO3", "WR", 
"CS", "OE", "ADD_EN"};
................
gpio_init() {

     int i;
     for (i = 0; i < GPIOS_NUM; ++i) {
         syslog(LOG_INFO,"%s %d",__FUNCTION__,__LINE__);
         gpio_lines[i] = gpiod_line_find(gpio_names[i]);
         syslog(LOG_INFO,"%s %d",__FUNCTION__,__LINE__);
     }
}
-------------------------------------------
I used syslog/printk to measure time.
syslog in example shows about 300ms on each gpiod_line_find call.
17.07.2020 16:37, Linus Walleij пишет:
> Hi Maxim,
> 
> On Fri, Jul 17, 2020 at 2:56 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> 
>> I'm using libgpiod in userspace.
>> I have 6 gpiochip's on my board.
>> gpiod_line_find takes about 300ms to find GPIO line.
>>
>> gpiod_line_find calls gpiod_foreach_chip
>> then gpiod_chip_iter_next
>> then gpiod_chip_close then close(chip->fd)
>> then we are going to kernel gpiolib gpio_chrdev_release
>> then atomic_notifier_chain_unregister
>> then synchronize_rcu()
>>
>> synchronize_rcu takes about 30 ms (6*30ms=280ms)
>>
>> I tried to remove synchronize_rcu from atomic_notifier_chain_unregister
>> and gpiod_line_find takes about 2ms now.
> 
> Interesting! Can you provide some context? Are you just testing because
> curious or do you need to meet a design objective?
> 
> Did you use ftrace or similar instrumentation to drill down and find
> where time is spent?
> 
> Yours,
> Linus Walleij
> 

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Maxim Kochetkov]

I need a small userspace program to do some GPIO magic to communicate 
other hardware like devmem. This program takes about 2,5 seconds just to 
find GPIO lines by name.

replacing synchronize_rcu to synchronize_rcu_expedited in 
atomic_notifier_chain_unregister gives the same boost as removing 
synchronize_rcu

17.07.2020 16:37, Linus Walleij пишет:
> Are you just testing because
> curious or do you need to meet a design objective?

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Andy Shevchenko]

On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
>
> I need a small userspace program to do some GPIO magic to communicate
> other hardware like devmem. This program takes about 2,5 seconds just to
> find GPIO lines by name.
>
> replacing synchronize_rcu to synchronize_rcu_expedited in
> atomic_notifier_chain_unregister gives the same boost as removing
> synchronize_rcu

Have you tried to replace an atomic notifier call with a regular one?
IIRC it's still not clear why atomic is used there.

-- 
With Best Regards,
Andy Shevchenko

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Kent Gibson]

On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> >
> > I need a small userspace program to do some GPIO magic to communicate
> > other hardware like devmem. This program takes about 2,5 seconds just to
> > find GPIO lines by name.
> >
> > replacing synchronize_rcu to synchronize_rcu_expedited in
> > atomic_notifier_chain_unregister gives the same boost as removing
> > synchronize_rcu
> 
> Have you tried to replace an atomic notifier call with a regular one?
> IIRC it's still not clear why atomic is used there.
> 

Indeed, I recently submitted a patch to switch the
atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
the chained calls can sleep.
Not sure if that is related, or if the change would make this case better
or worse, but it would be interesting to find out.
The patch is in the current gpio/devel, btw.

Cheers,
Kent.

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Maxim Kochetkov]

It works fine with this patch. Thank you so much!

18.07.2020 07:25, Kent Gibson пишет:
> On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
>> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
>>>
>>> I need a small userspace program to do some GPIO magic to communicate
>>> other hardware like devmem. This program takes about 2,5 seconds just to
>>> find GPIO lines by name.
>>>
>>> replacing synchronize_rcu to synchronize_rcu_expedited in
>>> atomic_notifier_chain_unregister gives the same boost as removing
>>> synchronize_rcu
>>
>> Have you tried to replace an atomic notifier call with a regular one?
>> IIRC it's still not clear why atomic is used there.
>>
> 
> Indeed, I recently submitted a patch to switch the
> atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
> the chained calls can sleep.
> Not sure if that is related, or if the change would make this case better
> or worse, but it would be interesting to find out.
> The patch is in the current gpio/devel, btw.
> 
> Cheers,
> Kent.
> 

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Bartosz Golaszewski]

On Mon, Jul 20, 2020 at 10:14 AM Maxim Kochetkov <fido_max@inbox.ru> wrote:
>
> It works fine with this patch. Thank you so much!
>
> 18.07.2020 07:25, Kent Gibson пишет:
> > On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
> >> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> >>>
> >>> I need a small userspace program to do some GPIO magic to communicate
> >>> other hardware like devmem. This program takes about 2,5 seconds just to
> >>> find GPIO lines by name.
> >>>
> >>> replacing synchronize_rcu to synchronize_rcu_expedited in
> >>> atomic_notifier_chain_unregister gives the same boost as removing
> >>> synchronize_rcu
> >>
> >> Have you tried to replace an atomic notifier call with a regular one?
> >> IIRC it's still not clear why atomic is used there.
> >>
> >
> > Indeed, I recently submitted a patch to switch the
> > atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
> > the chained calls can sleep.
> > Not sure if that is related, or if the change would make this case better
> > or worse, but it would be interesting to find out.
> > The patch is in the current gpio/devel, btw.
> >
> > Cheers,
> > Kent.
> >

Linus,

I think we should consider submitting this patch for stable then
because this slow-down was affects previously existing use-cases.

Bartosz

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Kent Gibson]

On Fri, Jul 24, 2020 at 09:36:03PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jul 20, 2020 at 10:14 AM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> >
> > It works fine with this patch. Thank you so much!
> >
> > 18.07.2020 07:25, Kent Gibson пишет:
> > > On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
> > >> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> > >>>
> > >>> I need a small userspace program to do some GPIO magic to communicate
> > >>> other hardware like devmem. This program takes about 2,5 seconds just to
> > >>> find GPIO lines by name.
> > >>>
> > >>> replacing synchronize_rcu to synchronize_rcu_expedited in
> > >>> atomic_notifier_chain_unregister gives the same boost as removing
> > >>> synchronize_rcu
> > >>
> > >> Have you tried to replace an atomic notifier call with a regular one?
> > >> IIRC it's still not clear why atomic is used there.
> > >>
> > >
> > > Indeed, I recently submitted a patch to switch the
> > > atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
> > > the chained calls can sleep.
> > > Not sure if that is related, or if the change would make this case better
> > > or worse, but it would be interesting to find out.
> > > The patch is in the current gpio/devel, btw.
> > >
> > > Cheers,
> > > Kent.
> > >
> 
> Linus,
> 
> I think we should consider submitting this patch for stable then
> because this slow-down was affects previously existing use-cases.
> 

That patch is post the cdev split, so it certainly wont apply to an
older kernel.  I'm happy to backport it if you need it - just
nominate the branch you want it for.

Cheers,
Kent.

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Bartosz Golaszewski]

On Sat, Jul 25, 2020 at 5:52 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 09:36:03PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jul 20, 2020 at 10:14 AM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> > >
> > > It works fine with this patch. Thank you so much!
> > >
> > > 18.07.2020 07:25, Kent Gibson пишет:
> > > > On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
> > > >> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> > > >>>
> > > >>> I need a small userspace program to do some GPIO magic to communicate
> > > >>> other hardware like devmem. This program takes about 2,5 seconds just to
> > > >>> find GPIO lines by name.
> > > >>>
> > > >>> replacing synchronize_rcu to synchronize_rcu_expedited in
> > > >>> atomic_notifier_chain_unregister gives the same boost as removing
> > > >>> synchronize_rcu
> > > >>
> > > >> Have you tried to replace an atomic notifier call with a regular one?
> > > >> IIRC it's still not clear why atomic is used there.
> > > >>
> > > >
> > > > Indeed, I recently submitted a patch to switch the
> > > > atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
> > > > the chained calls can sleep.
> > > > Not sure if that is related, or if the change would make this case better
> > > > or worse, but it would be interesting to find out.
> > > > The patch is in the current gpio/devel, btw.
> > > >
> > > > Cheers,
> > > > Kent.
> > > >
> >
> > Linus,
> >
> > I think we should consider submitting this patch for stable then
> > because this slow-down was affects previously existing use-cases.
> >
>
> That patch is post the cdev split, so it certainly wont apply to an
> older kernel.  I'm happy to backport it if you need it - just
> nominate the branch you want it for.
>
> Cheers,
> Kent.

Linus' devel branch is fine - I haven't picked up new patches in past weeks.

Bartosz

Re: gpiolib gpio_chrdev_release duration is about 30 ms

[Linus Walleij]

On Sat, Jul 25, 2020 at 5:52 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Fri, Jul 24, 2020 at 09:36:03PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jul 20, 2020 at 10:14 AM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> > >
> > > It works fine with this patch. Thank you so much!
> > >
> > > 18.07.2020 07:25, Kent Gibson пишет:
> > > > On Fri, Jul 17, 2020 at 06:07:04PM +0300, Andy Shevchenko wrote:
> > > >> On Fri, Jul 17, 2020 at 5:17 PM Maxim Kochetkov <fido_max@inbox.ru> wrote:
> > > >>>
> > > >>> I need a small userspace program to do some GPIO magic to communicate
> > > >>> other hardware like devmem. This program takes about 2,5 seconds just to
> > > >>> find GPIO lines by name.
> > > >>>
> > > >>> replacing synchronize_rcu to synchronize_rcu_expedited in
> > > >>> atomic_notifier_chain_unregister gives the same boost as removing
> > > >>> synchronize_rcu
> > > >>
> > > >> Have you tried to replace an atomic notifier call with a regular one?
> > > >> IIRC it's still not clear why atomic is used there.
> > > >>
> > > >
> > > > Indeed, I recently submitted a patch to switch the
> > > > atomic_notifier_call_chain to blocking_notifier_call_chain, as some of
> > > > the chained calls can sleep.
> > > > Not sure if that is related, or if the change would make this case better
> > > > or worse, but it would be interesting to find out.
> > > > The patch is in the current gpio/devel, btw.
> > > >
> > > > Cheers,
> > > > Kent.
> > > >
> >
> > Linus,
> >
> > I think we should consider submitting this patch for stable then
> > because this slow-down was affects previously existing use-cases.
> >
>
> That patch is post the cdev split, so it certainly wont apply to an
> older kernel.  I'm happy to backport it if you need it - just
> nominate the branch you want it for.

Let's let this land upstream in the merge window and then we can consider
backports for performance regressions.

Yours,
Linus Walleij