From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage Date: Wed, 22 Nov 2017 16:13:31 -0800 Message-ID: <87tvxlzwb8.fsf@anholt.net> References: <20171122203928.28135-1-boris.brezillon@free-electrons.com> <8760a2ko9z.fsf@anholt.net> <20171122222850.075db871@bbrezillon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1867671761==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 198F66E6F2 for ; Thu, 23 Nov 2017 00:13:34 +0000 (UTC) In-Reply-To: <20171122222850.075db871@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: David Airlie , Stefan Wahren , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1867671761== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > On Wed, 22 Nov 2017 13:16:08 -0800 > Eric Anholt wrote: > >> Boris Brezillon writes: >>=20 >> > With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's >> > passed a refcount object that has its counter set to 0. In this driver, >> > this is a valid use case since we want to increment ->usecnt only when >> > the BO object starts to be used by real HW components and this is >> > definitely not the case when the BO is created. >> > >> > Fix the problem by using refcount_inc_not_zero() instead of >> > refcount_inc() and fallback to refcount_set(1) when >> > refcount_inc_not_zero() returns false. Note that this 2-steps operation >> > is not racy here because the whole section is protected by a mutex >> > which guarantees that the counter does not change between the >> > refcount_inc_not_zero() and refcount_set() calls.=20=20 >>=20 >> If we're not following the model, and protecting the refcount by a >> mutex, shouldn't we just be using addition and subtraction instead of >> refcount's atomics? > > Actually, this mutex is protecting the bo->madv value which has to be > checked when the counter reaches 0 (when decrementing) or 1 (when > incrementing). We just benefit from this protection here, but ideally > it would be better to have an refcount_inc_allow_zero() as suggested by > Daniel. Let me restate this to see if it makes sense: The refcount is always >=3D 0, this is is the only path that increases the refcount from 0 to 1, and it's (incidentally) protected by a mutex, so there's no race between the attempted increase from nonzero and the set from nonzero to 1. This seems fine to me as a bugfix. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAloWEqsACgkQtdYpNtH8 nuiqUA/8DeRZvT9PfLRN9eyzhzDnuWef2UE/TAeZOLOrNVglsV3ufyvrfnaZiOTa jlpWeIm0CLjwDXml5DQGfA8KHW3/XbqUKToiAerwWeBiCX4HeDUqDJPpujTiUOz1 +Az9US1uD3OQIcUo8VvhUpnlOuoZtjWwWcqkJYGsc/8F5LBzKk1azNo8yfu2m4nQ RfLBiLxasLVCev5TUzCYeZPH+QlzrDYISfVHTI1ctp19KxryiWfkYSGqMQskQgrn F4VnirYLiHcWbE1tg/OLtWAJocmjFodwjaaVckhbVJYwIJlec2xZSThxAP1LBYr6 nH+0zvXk2CTLb6LSjYQu6lDmtuegX2DpkqE9f955Xd30TkIkBk+jZTpy2CqZYAPZ 7IdO5aroOTLXpmN0U6GMLbrpPlkMx5fYDvDxr5M+0LJHINTqeDX4OLnASf9mA2ep N29531x4aUSgdqhaCD6N+n+FS+7YPjtd4z7BdqhvbxC4uo1nLojh6HebmikD478m /eUkUV0djZPWU1xE2WH9a8PbKNlC5GWFE02bgwEvUoP7dxYKr4Eg0s83TGMD7h3E CPEggYy2ToL473ispV/bDV3Ju9kU3Jt7NvTDzwsBzz9c2p+j7XJzHcxwowvQltLk O7VWxbT9bcUrYfku5YQEevPYLAQQgRqH50rPN0ZxJ8TTSwbNzyE= =SYDI -----END PGP SIGNATURE----- --=-=-=-- --===============1867671761== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1867671761==--