Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752140AbbERCHJ (ORCPT ); Sun, 17 May 2015 22:07:09 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35243 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751930AbbERCHC (ORCPT ); Sun, 17 May 2015 22:07:02 -0400 Date: Mon, 18 May 2015 12:06:47 +1000 From: NeilBrown To: Patrick Marlier Cc: Steven Rostedt , "Paul E. McKenney" , linux-kernel@vger.kernel.org, mingo@kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com, wangyun@linux.vnet.ibm.com Subject: Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage Message-ID: <20150518120647.0c3cecd8@notabene.brown> In-Reply-To: <5557819E.1060001@gmail.com> References: <20150512224603.GA4531@linux.vnet.ibm.com> <1431470787-4702-1-git-send-email-paulmck@linux.vnet.ibm.com> <1431470787-4702-3-git-send-email-paulmck@linux.vnet.ibm.com> <20150512223853.55e0ed90@grimm.local.home> <20150513125839.371ef677@notabene.brown> <5557819E.1060001@gmail.com> X-Mailer: Claws Mail 3.10.1-162-g4d0ed6 (GTK+ 2.24.25; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/DWv4GpK/QPK.y=MFGhpT8xf"; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6227 Lines: 192 --Sig_/DWv4GpK/QPK.y=MFGhpT8xf Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Sat, 16 May 2015 19:42:54 +0200 Patrick Marlier wrote: >=20 >=20 > On 05/13/2015 04:58 AM, NeilBrown wrote: > > On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt = wrote: > > > >> On Tue, 12 May 2015 15:46:26 -0700 > >> "Paul E. McKenney" wrote: > >> > >>> From: Patrick Marlier > >>> > >>> Signed-off-by: Patrick Marlier > >>> Signed-off-by: Paul E. McKenney > >>> --- > >>> drivers/md/bitmap.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c > >>> index 2bc56e2a3526..32901772e4ee 100644 > >>> --- a/drivers/md/bitmap.c > >>> +++ b/drivers/md/bitmap.c > >>> @@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md= _rdev *rdev, struct mddev *mdde > >>> rcu_read_lock(); > >>> if (rdev =3D=3D NULL) > >>> /* start at the beginning */ > >>> - rdev =3D list_entry_rcu(&mddev->disks, struct md_rdev, same_set); > >>> + rdev =3D list_entry_rcu(mddev->disks.next, struct md_rdev, same_se= t); > >> > >> Hmm, this changes the semantics. > >> > >> The original code looks nasty, I first thought it was broken, but it > >> seems to work out of sheer luck (or clever hack) > > > > Definitely a clever hack - no question of "luck" here :-) > > > > It might makes sense to change it to use list_for_each_entry_from_rcu() > > > > if (rdev =3D=3D NULL) > > rdev =3D list_entry_rcu(mddev->disks.next, struct md_rdev, same_s= et); > > else { > > rdev_dec_pending(rdev, mddev); > > rdev =3D list_next_entry_rcu(rdev->same_set.next, struct md_rdev,= same_set); > > } > > list_for_each_entry_from_rcu(rdev, ....) > > > > but there isn't a "list_next_entry_rcu".... > > > > > > Also, it would have been polity to at least 'cc' them Maintainer of thi= s code > > in the original patch - no? >=20 > Sure my bad. I hesitated to CC maintainers. I was almost sure that it=20 > will be rejected so I wanted to avoid noise. Well... If the subject has contained the magic string "RFC" I might have be= en less concerned. But there have been enough times that people have changed md without telling me, and thereby broken it, that I'd much rather see the patch than not. >=20 >=20 > > > > Thanks, > > NeilBrown > > > >> > >>> else { > >>> /* release the previous rdev and start from there. */ > >>> rdev_dec_pending(rdev, mddev); > >> > >> > >> What comes after this is: > >> > >> list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) { > >> if (rdev->raid_disk >=3D 0 && > >> > >> Now the original code had: > >> > >> rdev =3D list_entry_rcu(&mddev->disks, struct md_rdev, same_set); > >> > >> Where &mddev->disks would return the address of the disks field of > >> mddev which is a list head. Then it would get the 'same_set' offset, > >> which is 0, and rdev is pointing to a makeshift md_rdev struct. But it > >> isn't used, as the list_for_each_entry_continue_rcu() has: > >> > >> #define list_for_each_entry_continue_rcu(pos, head, member) \ > >> for (pos =3D list_entry_rcu(pos->member.next, typeof(*pos), member); \ > >> &pos->member !=3D (head); \ > >> pos =3D list_entry_rcu(pos->member.next, typeof(*pos), member)) > >> > >> Thus the first use of pos is pos->member.next or: > >> > >> mddev->disks.next > >> > >> But now you converted it to rdev =3D mddev->disks.next, which means the > >> first use is: > >> > >> pos =3D mddev->disks.next->next > >> > >> I think you are skipping the first element here. >=20 >=20 > struct mddev { > ... > struct list_head disks; > ...} >=20 > struct list_head { > struct list_head *next, *prev; > }; >=20 > The tricky thing is that "list_entry_rcu" before and after the patch is=20 > reading the same thing. No it isn't. Before the patch it is passed the address of the 'next' field. After the patch it is passed the contents of the 'next' field. >=20 > However in your case, the change I proposed is probably wrong I trust=20 > you on this side. :) What's your proposal to fix it with the rculist patc= h? What needs fixing? I don't see anything broken. Maybe there is something in this "rculist patch" that I'm missing. Can you point me at it? Thanks, NeilBrown >=20 > PS: In the rculist patch I proposed, I avoid the store and the atomic=20 > reload in the stack variable __ptr. (yeap, the=20 > rcu_dereference_raw/ACCESS_ONCE is a bit confusing because it implicitly= =20 > do & on the parameter). >=20 > Thanks. > -- > Pat > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ --Sig_/DWv4GpK/QPK.y=MFGhpT8xf Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIVAwUBVVlJNznsnt1WYoG5AQJoPg//TJigLyDYO58Z0dAYvGPOlrCb1gwk1hWp a2v1Coc7S21rWb/7qxv6xD0PIpH7Y4J2rTifihYOAaQBvllzV0g8caJ/ubJoU7Gy Qlh74cXUnOeUvSX5vGb0A4RKlH8bGU9MHlur8x6uZe1Wy2nnUYZzEEvVm44P0lKv 3ObcK2t7Np3qLfa2iE25OMnkWQQd+UeU3saewQyNCIPpfY93UlUE8jvLBpTaiPS1 rzAO0E76QjoUjCZfc9VaWMmZm/EXtpFIZix4+WyLDlieTehr/kXYLYAOGLQwSqdI 24+h/dwkCNnfCFvKjGotitx7RdsIjojsMjgDryHn5CJkKKs589cx10uQB9UwjYQV nAdBU6N1nsYHT9xe/88WbZ+qAkElykDdzWsZEHHJq2PFHrHHqRokgfmqgRn6myZi HXWa0qARH/GbRq8xF+GGbVWzlwXpCsWCpZsUHiRRl131BcRfXJ/kt+1/m7f895DZ D02IxK7VHFyvLFKCu9N5k3rzXs04oXwWWRZz+qR6HZqQ7QPnahv7w6ha3aPj51cF k7t7EA2kOI15/P9fKwHQAxCYFyhYs8I2gMlEHx9/PWY3dn/n3lmtI3clx14qcnJa OFefQrXv2e7WY25f6Gjkq9q56rZxXwtAOZNp+AaTnbfJ7281t51zKxu76DcrEKtE Kg1iJ7mWUuo= =sCWw -----END PGP SIGNATURE----- --Sig_/DWv4GpK/QPK.y=MFGhpT8xf-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/