Linux-GPIO Archive mirror
 help / color / mirror / Atom feed

From: Bartosz Golaszewski <brgl@bgdev.pl>
To: "Paul E . McKenney" <paulmck@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	 Kent Gibson <warthog618@gmail.com>,
	Neil Armstrong <neil.armstrong@linaro.org>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Performance regression in GPIOLIB with SRCU when using the user-space ABI in a *wrong* way
Date: Mon, 6 May 2024 14:32:57 +0200	[thread overview]
Message-ID: <CAMRc=Mfig2oooDQYTqo23W3PXSdzhVO4p=G4+P8y1ppBOrkrJQ@mail.gmail.com> (raw)

Hi Paul et al,

I have recently received two independent reports of a performance
regression in the GPIO character device. In both cases it was bisected
to commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with
SRCU"). I assume it's caused by the wait imposed by synchronize_srcu()
in desc_set_label().

In both cases the drop in performance is caused by the fact that the
user-space code in question does something like this (treat it as
pseudocode as in both cases users were using libgpiod v1.6.x but the
result with libgpiod v2 would be the same):

    for (;;) {
        req = gpiod_chip_request_lines(chip, ...);
        gpiod_line_request_get/set_value(req, ...);
        gpiod_line_request_release(req);
    }

This pattern can be seen in code that implements some bitbanging
protocols etc. from user-space - programs that call request/release in
quick succession can indeed see a slow-down now with SRCU.

Please note that this is almost certainly wrong: in general the user
process should request the GPIO, keep it requested and then perform
any accesses as required. What this code does, would be analogous to
the following code in the kernel:

    for (;;) {
        desc = gpiod_get(dev, "foo", ...);
        gpiod_set_value(desc, ...);
        gpiod_put(desc);
    }

Of course what drivers actually do is: call gpiod_get() once in
probe() and free the descriptor with gpiod_put() in remove() and
user-space should follow the same pattern.

While I could just brush it off and tell the users to fix their code
as the libgpiod test-suite which executes a more realistic set of
operations actually runs slightly faster after the recent rework, I
assume I'll be getting more reports like this so I want to look into
it and see if something can be done.

It turns out that performance stays the same if the thread running the
above code is pinned to a single CPU (for example: using
pthread_setaffinity_np()). Is this normal for SRCU to wait for much
longer if this is not the case? I don't know enough to understand why
this is the case.

The offending kernel code looks like this:

    old = rcu_replace_pointer(desc->label, new, 1);
    synchronize_srcu(&desc->srcu);
    kfree_const(old);

I was wondering if we even have to synchronize here? The corresponding
read-only sections call srcu_dereference(desc->label, &desc->srcu).
Would it be enough to implement kfree_const_rcu() and use it here
without synchronizing? Would the read-only users correctly see that
last dereferenced address still points to valid memory until they all
release the lock and the memory would only then be freed? Is my
understanding of kfree_rcu() correct?

Bart

next             reply	other threads:[~2024-05-06 12:33 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-06 12:32 Bartosz Golaszewski [this message]
2024-05-06 13:55 ` Performance regression in GPIOLIB with SRCU when using the user-space ABI in a *wrong* way Paul E. McKenney
2024-05-06 16:34   ` Bartosz Golaszewski
2024-05-06 17:01     ` Paul E. McKenney
2024-05-06 17:46       ` Bartosz Golaszewski
2024-05-06 18:07         ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMRc=Mfig2oooDQYTqo23W3PXSdzhVO4p=G4+P8y1ppBOrkrJQ@mail.gmail.com' \
    --to=brgl@bgdev.pl \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=neil.armstrong@linaro.org \
    --cc=paulmck@kernel.org \
    --cc=warthog618@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).