Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Jul 2019 13:30:02 +0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Yuyang Du <duyuyang@gmail.com>
Cc:     peterz@infradead.org, will.deacon@arm.com, mingo@kernel.org,
        bvanassche@acm.org, ming.lei@redhat.com, frederic@kernel.org,
        tglx@linutronix.de, linux-kernel@vger.kernel.org,
        longman@redhat.com, paulmck@linux.vnet.ibm.com
Subject: Re: [PATCH v3 30/30] locking/lockdep: Remove irq-safe to irq-unsafe
 read check
Message-ID: <20190710053002.GC14490@tardis>
References: <20190628091528.17059-1-duyuyang@gmail.com>
 <20190628091528.17059-31-duyuyang@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="H8ygTp4AXg6deix2"
Content-Disposition: inline
In-Reply-To: <20190628091528.17059-31-duyuyang@gmail.com>
User-Agent: Mutt/1.12.0 (2019-05-25)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--H8ygTp4AXg6deix2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 28, 2019 at 05:15:28PM +0800, Yuyang Du wrote:
> We have a lockdep warning:
>=20
>   =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
>   WARNING: possible irq lock inversion dependency detected
>   5.1.0-rc7+ #141 Not tainted
>   --------------------------------------------------------
>   kworker/8:2/328 just changed the state of lock:
>   0000000007f1a95b (&(&host->lock)->rlock){-...}, at: ata_bmdma_interrupt=
+0x27/0x1c0 [libata]
>   but this lock took another, HARDIRQ-READ-unsafe lock in the past:
>    (&trig->leddev_list_lock){.+.?}
>=20
> and interrupts could create inverse lock ordering between them.
>=20
> other info that might help us debug this:
>    Possible interrupt unsafe locking scenario:
>=20
>          CPU0                    CPU1
>          ----                    ----
>     lock(&trig->leddev_list_lock);
>                                  local_irq_disable();
>                                  lock(&(&host->lock)->rlock);
>                                  lock(&trig->leddev_list_lock);
>     <Interrupt>
>       lock(&(&host->lock)->rlock);
>=20
>  *** DEADLOCK ***
>=20
> This splat is a false positive, which is enabled by the addition of

If so, I think the better way is to reorder this patch before recursive
read lock suppport, for better bisect-ability.

Regards,
Boqun

> recursive read locks in the graph. Specifically, trig->leddev_list_lock i=
s a
> rwlock_t type, which was not in the graph before recursive read lock supp=
ort
> was added in lockdep.
>=20
> This false positve is caused by a "false-positive" check in IRQ usage che=
ck.
>=20
> In mark_lock_irq(), the following checks are currently performed:
>=20
>    ----------------------------------
>   |   ->      | unsafe | read unsafe |
>   |----------------------------------|
>   | safe      |  F  B  |    F* B*    |
>   |----------------------------------|
>   | read safe |  F* B* |      -      |
>    ----------------------------------
>=20
> Where:
> F: check_usage_forwards
> B: check_usage_backwards
> *: check enabled by STRICT_READ_CHECKS
>=20
> But actually the safe -> unsafe read dependency does not create a deadlock
> scenario.
>=20
> Fix this by simply removing those two checks, and since safe read -> unsa=
fe
> is indeed a problem, these checks are not actually strict per se, so remo=
ve
> the macro STRICT_READ_CHECKS, and we have the following checks:
>=20
>    ----------------------------------
>   |   ->      | unsafe | read unsafe |
>   |----------------------------------|
>   | safe      |  F  B  |      -      |
>   |----------------------------------|
>   | read safe |  F  B  |      -      |
>    ----------------------------------
>=20
> Signed-off-by: Yuyang Du <duyuyang@gmail.com>
> ---
>  kernel/locking/lockdep.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>=20
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index c7ba647..d12ab0e 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3558,8 +3558,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
>  	return 0;
>  }
> =20
> -#define STRICT_READ_CHECKS	1
> -
>  static int (*state_verbose_f[])(struct lock_class *class) =3D {
>  #define LOCKDEP_STATE(__STATE) \
>  	__STATE##_verbose,
> @@ -3605,7 +3603,7 @@ typedef int (*check_usage_f)(struct task_struct *, =
struct held_lock *,
>  	 * Validate that the lock dependencies don't have conflicting usage
>  	 * states.
>  	 */
> -	if ((!read || STRICT_READ_CHECKS) &&
> +	if ((!read || !dir) &&
>  			!usage(curr, this, excl_bit, state_name(new_bit & ~LOCK_USAGE_READ_MA=
SK)))
>  		return 0;
> =20
> @@ -3616,7 +3614,7 @@ typedef int (*check_usage_f)(struct task_struct *, =
struct held_lock *,
>  		if (!valid_state(curr, this, new_bit, excl_bit + LOCK_USAGE_READ_MASK))
>  			return 0;
> =20
> -		if (STRICT_READ_CHECKS &&
> +		if (dir &&
>  			!usage(curr, this, excl_bit + LOCK_USAGE_READ_MASK,
>  				state_name(new_bit + LOCK_USAGE_READ_MASK)))
>  			return 0;
> --=20
> 1.8.3.1
>=20

--H8ygTp4AXg6deix2
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAl0ld9cACgkQSXnow7UH
+rgUsAf9HKSkatoqAeAKDWKIc9pqAIuJD0lGuWOXjqeHMfCZgCkqdnUKecBq4M/k
QyY6QUrzscvr5+7nNJvRkUFDkXtN4sUguty+ig5WW8LCR9NN+X3ZY+ElSAVUzAsV
qrXQnaDaW0o4SRpkwNnFpQLU8V+33yHPxehXbViE4gruarYutcD9shyGrKkNgHhR
lSw2a703Ct5zFTqTKwnJgc8WYZUVA7AN5IGDSaIG1+a2ez25PPug7d61XmhFmlsh
t+w9PZjeFjywtIKoktr6zkCDyYmSjkgH4p5bGGYUKi5DE8XZ0+HIPv2YSPZ09ABE
74SfFMXBgdATqiYqtT3kud3AvaCwCQ==
=tMGO
-----END PGP SIGNATURE-----

--H8ygTp4AXg6deix2--