Date: Mon, 18 Feb 2013 19:09:57 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Svatopluk Kraus <onwahe@gmail.com> Cc: freebsd-current@freebsd.org Subject: Re: [patch] i386 pmap sysmaps_pcpu[] atomic access Message-ID: <20130218170957.GJ2598@kib.kiev.ua> In-Reply-To: <CAFHCsPVbkwj7fhqax5D5kk89VZgAjW9gT8uJunjevav2eTUbNQ@mail.gmail.com> References: <CAFHCsPUVTM9jfrnzY72YsPszLWkg-UaJcycTR4xXcS%2BfPzS1Vg@mail.gmail.com> <20130218150809.GG2598@kib.kiev.ua> <CAFHCsPVbkwj7fhqax5D5kk89VZgAjW9gT8uJunjevav2eTUbNQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--tKy6e3LXpfmanBFM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Feb 18, 2013 at 06:06:42PM +0100, Svatopluk Kraus wrote:
> On Mon, Feb 18, 2013 at 4:08 PM, Konstantin Belousov
> <kostikbel@gmail.com> wrote:
> > On Mon, Feb 18, 2013 at 01:44:35PM +0100, Svatopluk Kraus wrote:
> >> Hi,
> >>
> >> the access to sysmaps_pcpu[] should be atomic with respect to
> >> thread migration. Otherwise, a sysmaps for one CPU can be stolen by
> >> another CPU and the purpose of per CPU sysmaps is broken. A patch is
> >> enclosed.
> > And, what are the problem caused by the 'otherwise' ?
> > I do not see any.
>=20
> The 'otherwise' issue is the following:
>=20
> 1. A thread is running on CPU0.
>=20
> sysmaps =3D &sysmaps_pcpu[PCPU_GET(cpuid)];
>=20
> 2. A sysmaps variable contains a pointer to 'CPU0' sysmaps.
> 3. Now, the thread migrates into CPU1.
> 4. However, the sysmaps variable still contains a pointers to 'CPU0' sysm=
aps.
>=20
> mtx_lock(&sysmaps->lock);
>=20
> 4. The thread running on CPU1 locked 'CPU0' sysmaps mutex, so the
> thread uselessly can block another thread running on CPU0. Maybe, it's
> not a problem. However, it definitely goes against the reason why the
> submaps (one for each CPU) exist.
So what ?
>=20
>=20
> > Really, taking the mutex while bind to a CPU could be deadlock-prone
> > under some situations.
> >
> > This was discussed at least one more time. Might be, a comment saying t=
hat
> > there is no issue should be added.
>=20
> I missed the discussion. Can you point me to it, please? A deadlock is
> not problem here, however, I can be wrong, as I can't imagine now how
> a simple pinning could lead into a deadlock at all.
Because some other load on the bind cpu might prevent the thread from
being scheduled.
>=20
> >>
> >> Svata
> >>
> >> Index: sys/i386/i386/pmap.c
> >> =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=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- sys/i386/i386/pmap.c (revision 246831)
> >> +++ sys/i386/i386/pmap.c (working copy)
> >> @@ -4146,11 +4146,11 @@
> >> {
> >> struct sysmaps *sysmaps;
> >>
> >> + sched_pin();
> >> sysmaps =3D &sysmaps_pcpu[PCPU_GET(cpuid)];
> >> mtx_lock(&sysmaps->lock);
> >> if (*sysmaps->CMAP2)
> >> panic("pmap_zero_page: CMAP2 busy");
> >> - sched_pin();
> >> *sysmaps->CMAP2 =3D PG_V | PG_RW | VM_PAGE_TO_PHYS(m) | PG_A | P=
G_M |
> >> pmap_cache_bits(m->md.pat_mode, 0);
> >> invlcaddr(sysmaps->CADDR2);
> >> @@ -4171,11 +4171,11 @@
> >> {
> >> struct sysmaps *sysmaps;
> >>
> >> + sched_pin();
> >> sysmaps =3D &sysmaps_pcpu[PCPU_GET(cpuid)];
> >> mtx_lock(&sysmaps->lock);
> >> if (*sysmaps->CMAP2)
> >> panic("pmap_zero_page_area: CMAP2 busy");
> >> - sched_pin();
> >> *sysmaps->CMAP2 =3D PG_V | PG_RW | VM_PAGE_TO_PHYS(m) | PG_A | P=
G_M |
> >> pmap_cache_bits(m->md.pat_mode, 0);
> >> invlcaddr(sysmaps->CADDR2);
> >> @@ -4220,13 +4220,13 @@
> >> {
> >> struct sysmaps *sysmaps;
> >>
> >> + sched_pin();
> >> sysmaps =3D &sysmaps_pcpu[PCPU_GET(cpuid)];
> >> mtx_lock(&sysmaps->lock);
> >> if (*sysmaps->CMAP1)
> >> panic("pmap_copy_page: CMAP1 busy");
> >> if (*sysmaps->CMAP2)
> >> panic("pmap_copy_page: CMAP2 busy");
> >> - sched_pin();
> >> invlpg((u_int)sysmaps->CADDR1);
> >> invlpg((u_int)sysmaps->CADDR2);
> >> *sysmaps->CMAP1 =3D PG_V | VM_PAGE_TO_PHYS(src) | PG_A |
> >> @@ -5072,11 +5072,11 @@
> >> vm_offset_t sva, eva;
> >>
> >> if ((cpu_feature & CPUID_CLFSH) !=3D 0) {
> >> + sched_pin();
> >> sysmaps =3D &sysmaps_pcpu[PCPU_GET(cpuid)];
> >> mtx_lock(&sysmaps->lock);
> >> if (*sysmaps->CMAP2)
> >> panic("pmap_flush_page: CMAP2 busy");
> >> - sched_pin();
> >> *sysmaps->CMAP2 =3D PG_V | PG_RW | VM_PAGE_TO_PHYS(m) |
> >> PG_A | PG_M | pmap_cache_bits(m->md.pat_mode, 0);
> >> invlcaddr(sysmaps->CADDR2);
> >> _______________________________________________
> >> freebsd-current@freebsd.org mailing list
> >> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> >> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.=
org"
--tKy6e3LXpfmanBFM
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iQIcBAEBAgAGBQJRImBlAAoJEJDCuSvBvK1B//oQAIMCufPgLM/CyH3EaA9ngDQb
RlNOrGI6IZD38h9RX5fLbdsa1kakWcWFMFy9qxqpdNzWEF7r7tYt0N9/D6rxM8hB
2uUNIxhyGLTA6ZOgHKufauUQ7GMP4Eo8wUsehI1/smH6EpxmuSr3OMe7tJIOxzIx
N1pveX1OZGixpojxQR6/zYIklZhbAIOuR6YYpO7mass8jm1XyxDWHSEGzE+x5Cdk
H2bFwtaarwrDp02KrculRlxIqs2tLq7eeSD6fnDrUj1jDmY471TeaaOAxOkqd2Uy
78tbh6VGnNfT8lE28gLCIH2P2YnNDLhjn28wFy23Ltf6N2USvDmvmHQRqI3GWIU8
msoEg8axsKt/cA7dDotZE+Cbiq7cmby+H2VxHmZzL+PsiH23NpGhF6V9qtxj5RaF
vS4XYnEuC4jWPPYIZouPDWiA8iWsnvcd2G521ZX6FSUhHcybS05vYbHr9QyJFEqw
sNOl687yEv+z9yj3qc+bNMpnI7EjAmY98DzcbVtHp88cv0zAqRSDMY2xQKP1XWxX
5ZahBjc8da68ZZF98E2XGBiiKCU4hzpQi+XkzPUUdJ5Cwq3cEhBxCaED1JmHtYWw
vTv004xPmfhDmtPcd9pMpE1GuK8N6qZP3xw/Yv1Os/8gA2pVcIT47xmVReOJEph7
vE1lYuc7Vrfst/KSKgFP
=nHOR
-----END PGP SIGNATURE-----
--tKy6e3LXpfmanBFM--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130218170957.GJ2598>