Bug 145385
| Summary: | [cpu] Logical processor cannot be disabled for some SMT-enabled Intel procs | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Enji Cooper <ngie> | ||||||||
| Component: | kern | Assignee: | Andriy Gapon <avg> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Only Me | ||||||||||
| Priority: | Normal | ||||||||||
| Version: | Unspecified | ||||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Enji Cooper
2010-04-04 21:10:08 UTC
Author: avg Date: Thu Apr 15 08:29:14 2010 New Revision: 206648 URL: http://svn.freebsd.org/changeset/base/206648 Log: scsi_cd: CD_FLAG_VALID_MEDIA is sufficient to set d_sectorsize and d_mediasize CD_FLAG_VALID_TOC is not required for setting those media properties. PR: kern/145385 Submitted by: Juergen Lock <nox@jelal.kn-bremen.de> a slightly different version Tested by: Pavel Sukhoy <sukhoy@ripn.net>, Markus Wild <m.wild@cybernet.ch>, Juergen Lock <nox@jelal.kn-bremen.de>, uqs MFC after: 1 week Modified: head/sys/cam/scsi/scsi_cd.c Modified: head/sys/cam/scsi/scsi_cd.c ============================================================================== --- head/sys/cam/scsi/scsi_cd.c Thu Apr 15 08:20:57 2010 (r206647) +++ head/sys/cam/scsi/scsi_cd.c Thu Apr 15 08:29:14 2010 (r206648) @@ -2773,8 +2773,12 @@ cdcheckmedia(struct cam_periph *periph) softc->flags &= ~(CD_FLAG_VALID_MEDIA|CD_FLAG_VALID_TOC); cdprevent(periph, PR_ALLOW); return (error); - } else + } else { softc->flags |= CD_FLAG_VALID_MEDIA; + softc->disk->d_sectorsize = softc->params.blksize; + softc->disk->d_mediasize = + (off_t)softc->params.blksize * softc->params.disksize; + } /* * Now we check the table of contents. This (currently) is only @@ -2863,9 +2867,6 @@ cdcheckmedia(struct cam_periph *periph) } softc->flags |= CD_FLAG_VALID_TOC; - softc->disk->d_sectorsize = softc->params.blksize; - softc->disk->d_mediasize = - (off_t)softc->params.blksize * softc->params.disksize; bailout: _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" Huh...? That commit/patch has nothing to do with this PR :D. Thanks, -Garrett The following trivial patch fixes the issue on my W3520 processor; AFAICS it's what should be done after reading several of the specs because the logical count that's tracked with ebx is exactly what is needed for logical_cpus (it's an absolute quantity). I need to verify it with a multi-cpu topology at work (the two r710s I was testing with E-series Xeons on aren't available remotely right now). Thanks! -Garrett On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: > The following trivial patch fixes the issue on my W3520 processor; AFAICS it's what should be done after reading several of the specs because the logical count that's tracked with ebx is exactly what is needed for logical_cpus (it's an absolute quantity). I need to verify it with a multi-cpu topology at work (the two r710s I was testing with E-series Xeons on aren't available remotely right now). > Thanks! > -Garrett Jung-uk Kim and Attilio Rao have both been looking at this code recently and are in a better position to review the patch in the PR. -- John Baldwin On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: > On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >> =A0 =A0 =A0 The following trivial patch fixes the issue on my W3520 proc= essor; AFAICS > it's what should be done after reading several of the specs because the > logical count that's tracked with ebx is exactly what is needed for > logical_cpus (it's an absolute quantity). I need to verify it with a mult= i-cpu > topology at work (the two r710s I was testing with E-series Xeons on aren= 't > available remotely right now). >> Thanks! >> -Garrett > > Jung-uk Kim and Attilio Rao have both been looking at this code recently = and > are in a better position to review the patch in the PR. (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) The patch works as expected (it now properly detects the SMIT CPUs as logical CPUs), but setting machdep.hlt_logical_cpus=3D1 causes other problems with scheduling tasks because certain kernel threads get stuck at boot when netbooting (in particular I've seen problems with usbhub* and a few others bits), so in order for machdep.hlt_logical_cpus to be fixed on SMT processors, it might require some changes to the ULE scheduler to shuffle around the threads to available cores/processors? Thanks! -Garrett On Tue, 24 Aug 2010, Garrett Cooper wrote: > On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>> The following trivial patch fixes the issue on my W3520 processor; AFAICS >> it's what should be done after reading several of the specs because the >> logical count that's tracked with ebx is exactly what is needed for >> logical_cpus (it's an absolute quantity). I need to verify it with a multi-cpu >> topology at work (the two r710s I was testing with E-series Xeons on aren't >> available remotely right now). >>> Thanks! >>> -Garrett >> >> Jung-uk Kim and Attilio Rao have both been looking at this code recently and >> are in a better position to review the patch in the PR. > > (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) > > The patch works as expected (it now properly detects the SMIT CPUs as > logical CPUs), but setting machdep.hlt_logical_cpus=1 causes other > problems with scheduling tasks because certain kernel threads get > stuck at boot when netbooting (in particular I've seen problems with > usbhub* and a few others bits), so in order for > machdep.hlt_logical_cpus to be fixed on SMT processors, it might > require some changes to the ULE scheduler to shuffle around the > threads to available cores/processors? > hlt_logical_cpus should be rewritten to use cpusets to change the default system set rather than specifically halting those cpus. There are a number of loops in the kernel that iterate over all cpus and attempt to bind and perform some task. I think there are a number of other reasons to prefer a less aggressive approach to avoiding the logical cpus as well. Simply preventing user thread schedule will achieve the intent of the sysctl in any event. Thanks, Jeff > Thanks! > -Garrett > On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson <jroberson@jroberson.net> w= rote: > On Tue, 24 Aug 2010, Garrett Cooper wrote: > >> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>> >>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>>> >>>> =A0 =A0 =A0 The following trivial patch fixes the issue on my W3520 pr= ocessor; >>>> AFAICS >>> >>> it's what should be done after reading several of the specs because the >>> logical count that's tracked with ebx is exactly what is needed for >>> logical_cpus (it's an absolute quantity). I need to verify it with a >>> multi-cpu >>> topology at work (the two r710s I was testing with E-series Xeons on >>> aren't >>> available remotely right now). >>>> >>>> Thanks! >>>> -Garrett >>> >>> Jung-uk Kim and Attilio Rao have both been looking at this code recentl= y >>> and >>> are in a better position to review the patch in the PR. >> >> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >> >> The patch works as expected (it now properly detects the SMIT CPUs as >> logical CPUs), but setting machdep.hlt_logical_cpus=3D1 causes other >> problems with scheduling tasks because certain kernel threads get >> stuck at boot when netbooting (in particular I've seen problems with >> usbhub* and a few others bits), so in order for >> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >> require some changes to the ULE scheduler to shuffle around the >> threads to available cores/processors? >> > > hlt_logical_cpus should be rewritten to use cpusets to change the default > system set rather than specifically halting those cpus. =A0There are a nu= mber > of loops in the kernel that iterate over all cpus and attempt to bind and > perform some task. =A0I think there are a number of other reasons to pref= er a > less aggressive approach to avoiding the logical cpus as well. Simply > preventing user thread schedule will achieve the intent of the sysctl in = any > event. Ok... in that event then the bug is ok, but maybe I should add some code to the patch to warn the user about functional issues associated with halting logical CPUs? Thanks! -Garrett On Tue, 24 Aug 2010, Garrett Cooper wrote: > On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson <jroberson@jroberson.net> wrote: >> On Tue, 24 Aug 2010, Garrett Cooper wrote: >> >>> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>>> >>>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>>>> >>>>> The following trivial patch fixes the issue on my W3520 processor; >>>>> AFAICS >>>> >>>> it's what should be done after reading several of the specs because the >>>> logical count that's tracked with ebx is exactly what is needed for >>>> logical_cpus (it's an absolute quantity). I need to verify it with a >>>> multi-cpu >>>> topology at work (the two r710s I was testing with E-series Xeons on >>>> aren't >>>> available remotely right now). >>>>> >>>>> Thanks! >>>>> -Garrett >>>> >>>> Jung-uk Kim and Attilio Rao have both been looking at this code recently >>>> and >>>> are in a better position to review the patch in the PR. >>> >>> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >>> >>> The patch works as expected (it now properly detects the SMIT CPUs as >>> logical CPUs), but setting machdep.hlt_logical_cpus=1 causes other >>> problems with scheduling tasks because certain kernel threads get >>> stuck at boot when netbooting (in particular I've seen problems with >>> usbhub* and a few others bits), so in order for >>> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >>> require some changes to the ULE scheduler to shuffle around the >>> threads to available cores/processors? >>> >> >> hlt_logical_cpus should be rewritten to use cpusets to change the default >> system set rather than specifically halting those cpus. There are a number >> of loops in the kernel that iterate over all cpus and attempt to bind and >> perform some task. I think there are a number of other reasons to prefer a >> less aggressive approach to avoiding the logical cpus as well. Simply >> preventing user thread schedule will achieve the intent of the sysctl in any >> event. > > Ok... in that event then the bug is ok, but maybe I should add > some code to the patch to warn the user about functional issues > associated with halting logical CPUs? I don't think the bug is ok. We probably shouldn't have sysctls which readily break the kernel. As I said we should instead have the sysctl backend to cpuset. It shouldn't take more than an hour to code and test. Thanks, Jeff > Thanks! > -Garrett > On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: > On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: > > > On Tue, 24 Aug 2010, Garrett Cooper wrote: > > On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson <jroberson@jroberson.net> > wrote: > > On Tue, 24 Aug 2010, Garrett Cooper wrote: > > On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: > > On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: > > =A0 =A0 =A0 The following trivial patch fixes the issue on my W3520 proce= ssor; > > AFAICS > > it's what should be done after reading several of the specs because the > > logical count that's tracked with ebx is exactly what is needed for > > logical_cpus (it's an absolute quantity). I need to verify it with a > > multi-cpu > > topology at work (the two r710s I was testing with E-series Xeons on > > aren't > > available remotely right now). > > Thanks! > > -Garrett > > Jung-uk Kim and Attilio Rao have both been looking at this code recently > > and > > are in a better position to review the patch in the PR. > > (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) > > The patch works as expected (it now properly detects the SMIT CPUs as > > logical CPUs), but setting machdep.hlt_logical_cpus=3D1 causes other > > problems with scheduling tasks because certain kernel threads get > > stuck at boot when netbooting (in particular I've seen problems with > > usbhub* and a few others bits), so in order for > > machdep.hlt_logical_cpus to be fixed on SMT processors, it might > > require some changes to the ULE scheduler to shuffle around the > > threads to available cores/processors? > > > hlt_logical_cpus should be rewritten to use cpusets to change the default > > system set rather than specifically halting those cpus. =A0There are a nu= mber > > of loops in the kernel that iterate over all cpus and attempt to bind and > > perform some task. =A0I think there are a number of other reasons to pref= er a > > less aggressive approach to avoiding the logical cpus as well. Simply > > preventing user thread schedule will achieve the intent of the sysctl in = any > > event. > > =A0=A0Ok... in that event then the bug is ok, but maybe I should add > > some code to the patch to warn the user about functional issues > > associated with halting logical CPUs? > > I don't think the bug is ok. =A0We probably shouldn't have sysctls which > readily break the kernel. =A0As I said we should instead have the sysctl > backend to cpuset. =A0It shouldn't take more than an hour to code and tes= t. Ok.. I'll look at this once I have my other system back online so I can actively break something until I get it to work. Thanks, -Garrett On Tue, Aug 24, 2010 at 3:45 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote= : > On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> wrot= e: >> On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: >> >> >> On Tue, 24 Aug 2010, Garrett Cooper wrote: >> >> On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson <jroberson@jroberson.net= > >> wrote: >> >> On Tue, 24 Aug 2010, Garrett Cooper wrote: >> >> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >> >> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >> >> =A0 =A0 =A0 The following trivial patch fixes the issue on my W3520 proc= essor; >> >> AFAICS >> >> it's what should be done after reading several of the specs because the >> >> logical count that's tracked with ebx is exactly what is needed for >> >> logical_cpus (it's an absolute quantity). I need to verify it with a >> >> multi-cpu >> >> topology at work (the two r710s I was testing with E-series Xeons on >> >> aren't >> >> available remotely right now). >> >> Thanks! >> >> -Garrett >> >> Jung-uk Kim and Attilio Rao have both been looking at this code recently >> >> and >> >> are in a better position to review the patch in the PR. >> >> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >> >> The patch works as expected (it now properly detects the SMIT CPUs as >> >> logical CPUs), but setting machdep.hlt_logical_cpus=3D1 causes other >> >> problems with scheduling tasks because certain kernel threads get >> >> stuck at boot when netbooting (in particular I've seen problems with >> >> usbhub* and a few others bits), so in order for >> >> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >> >> require some changes to the ULE scheduler to shuffle around the >> >> threads to available cores/processors? >> >> >> hlt_logical_cpus should be rewritten to use cpusets to change the defaul= t >> >> system set rather than specifically halting those cpus. =A0There are a n= umber >> >> of loops in the kernel that iterate over all cpus and attempt to bind an= d >> >> perform some task. =A0I think there are a number of other reasons to pre= fer a >> >> less aggressive approach to avoiding the logical cpus as well. Simply >> >> preventing user thread schedule will achieve the intent of the sysctl in= any >> >> event. >> >> =A0=A0Ok... in that event then the bug is ok, but maybe I should add >> >> some code to the patch to warn the user about functional issues >> >> associated with halting logical CPUs? >> >> I don't think the bug is ok. =A0We probably shouldn't have sysctls which >> readily break the kernel. =A0As I said we should instead have the sysctl >> backend to cpuset. =A0It shouldn't take more than an hour to code and te= st. > > =A0 =A0Ok.. I'll look at this once I have my other system back online so > I can actively break something until I get it to work. BTW... there's a lot of code in machdep.c that does the same thing to idle the CPU, for instance, cpu_idle_hlt, cpu_idle_acpi, cpu_idle_amdc1e (on amd64). What should be done about those cases (same thing, or different)? Thanks, -Garrett On Tue, 24 Aug 2010, Garrett Cooper wrote: > On Tue, Aug 24, 2010 at 3:45 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: >> On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: >>> On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: >>> >>> >>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>> >>> On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson <jroberson@jroberson.net> >>> wrote: >>> >>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>> >>> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>> >>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>> >>> The following trivial patch fixes the issue on my W3520 processor; >>> >>> AFAICS >>> >>> it's what should be done after reading several of the specs because the >>> >>> logical count that's tracked with ebx is exactly what is needed for >>> >>> logical_cpus (it's an absolute quantity). I need to verify it with a >>> >>> multi-cpu >>> >>> topology at work (the two r710s I was testing with E-series Xeons on >>> >>> aren't >>> >>> available remotely right now). >>> >>> Thanks! >>> >>> -Garrett >>> >>> Jung-uk Kim and Attilio Rao have both been looking at this code recently >>> >>> and >>> >>> are in a better position to review the patch in the PR. >>> >>> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >>> >>> The patch works as expected (it now properly detects the SMIT CPUs as >>> >>> logical CPUs), but setting machdep.hlt_logical_cpus=1 causes other >>> >>> problems with scheduling tasks because certain kernel threads get >>> >>> stuck at boot when netbooting (in particular I've seen problems with >>> >>> usbhub* and a few others bits), so in order for >>> >>> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >>> >>> require some changes to the ULE scheduler to shuffle around the >>> >>> threads to available cores/processors? >>> >>> >>> hlt_logical_cpus should be rewritten to use cpusets to change the default >>> >>> system set rather than specifically halting those cpus. There are a number >>> >>> of loops in the kernel that iterate over all cpus and attempt to bind and >>> >>> perform some task. I think there are a number of other reasons to prefer a >>> >>> less aggressive approach to avoiding the logical cpus as well. Simply >>> >>> preventing user thread schedule will achieve the intent of the sysctl in any >>> >>> event. >>> >>> Ok... in that event then the bug is ok, but maybe I should add >>> >>> some code to the patch to warn the user about functional issues >>> >>> associated with halting logical CPUs? >>> >>> I don't think the bug is ok. We probably shouldn't have sysctls which >>> readily break the kernel. As I said we should instead have the sysctl >>> backend to cpuset. It shouldn't take more than an hour to code and test. >> >> Ok.. I'll look at this once I have my other system back online so >> I can actively break something until I get it to work. > > BTW... there's a lot of code in machdep.c that does the same thing > to idle the CPU, for instance, cpu_idle_hlt, cpu_idle_acpi, > cpu_idle_amdc1e (on amd64). What should be done about those cases > (same thing, or different)? Those are the actual idle functions that the scheduler uses. Those are safe. Thanks, Jeff > Thanks, > -Garrett > On Tue, Aug 24, 2010 at 9:53 PM, Jeff Roberson <jroberson@jroberson.net> wr= ote: > On Tue, 24 Aug 2010, Garrett Cooper wrote: > >> On Tue, Aug 24, 2010 at 3:45 PM, Garrett Cooper <yaneurabeya@gmail.com> >> wrote: >>> >>> On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> >>> wrote: >>>> >>>> On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: >>>> >>>> >>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>> >>>> On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson >>>> <jroberson@jroberson.net> >>>> wrote: >>>> >>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>> >>>> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>>> >>>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>>> >>>> =A0 =A0 =A0 The following trivial patch fixes the issue on my W3520 pr= ocessor; >>>> >>>> AFAICS >>>> >>>> it's what should be done after reading several of the specs because th= e >>>> >>>> logical count that's tracked with ebx is exactly what is needed for >>>> >>>> logical_cpus (it's an absolute quantity). I need to verify it with a >>>> >>>> multi-cpu >>>> >>>> topology at work (the two r710s I was testing with E-series Xeons on >>>> >>>> aren't >>>> >>>> available remotely right now). >>>> >>>> Thanks! >>>> >>>> -Garrett >>>> >>>> Jung-uk Kim and Attilio Rao have both been looking at this code recent= ly >>>> >>>> and >>>> >>>> are in a better position to review the patch in the PR. >>>> >>>> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >>>> >>>> The patch works as expected (it now properly detects the SMIT CPUs as >>>> >>>> logical CPUs), but setting machdep.hlt_logical_cpus=3D1 causes other >>>> >>>> problems with scheduling tasks because certain kernel threads get >>>> >>>> stuck at boot when netbooting (in particular I've seen problems with >>>> >>>> usbhub* and a few others bits), so in order for >>>> >>>> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >>>> >>>> require some changes to the ULE scheduler to shuffle around the >>>> >>>> threads to available cores/processors? >>>> >>>> >>>> hlt_logical_cpus should be rewritten to use cpusets to change the >>>> default >>>> >>>> system set rather than specifically halting those cpus. =A0There are a >>>> number >>>> >>>> of loops in the kernel that iterate over all cpus and attempt to bind >>>> and >>>> >>>> perform some task. =A0I think there are a number of other reasons to >>>> prefer a >>>> >>>> less aggressive approach to avoiding the logical cpus as well. Simply >>>> >>>> preventing user thread schedule will achieve the intent of the sysctl = in >>>> any >>>> >>>> event. >>>> >>>> =A0=A0Ok... in that event then the bug is ok, but maybe I should add >>>> >>>> some code to the patch to warn the user about functional issues >>>> >>>> associated with halting logical CPUs? >>>> >>>> I don't think the bug is ok. =A0We probably shouldn't have sysctls whi= ch >>>> readily break the kernel. =A0As I said we should instead have the sysc= tl >>>> backend to cpuset. =A0It shouldn't take more than an hour to code and >>>> test. >>> >>> =A0 =A0Ok.. I'll look at this once I have my other system back online s= o >>> I can actively break something until I get it to work. >> >> =A0 BTW... there's a lot of code in machdep.c that does the same thing >> to idle the CPU, for instance, cpu_idle_hlt, cpu_idle_acpi, >> cpu_idle_amdc1e (on amd64). What should be done about those cases >> (same thing, or different)? > > Those are the actual idle functions that the scheduler uses. =A0Those are > safe. I'll look into running this on a Nehalem processor machine, but this appears to as expected on my Penryn processor test machine with machdep.hlt_cpus =3D { 110, 101, 11, 0 } and with machdep.idle=3Dacpi; I'm not sure if the if the loop is supposed to be there still, but it wouldn't make sense because the CPU would be spinning in the kernel. Thanks, -Garrett On Wed, Aug 25, 2010 at 9:08 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: > On Tue, Aug 24, 2010 at 9:53 PM, Jeff Roberson <jroberson@jroberson.net> wrote: >> On Tue, 24 Aug 2010, Garrett Cooper wrote: >> >>> On Tue, Aug 24, 2010 at 3:45 PM, Garrett Cooper <yaneurabeya@gmail.com> >>> wrote: >>>> >>>> On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> >>>> wrote: >>>>> >>>>> On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: >>>>> >>>>> >>>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>>> >>>>> On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson >>>>> <jroberson@jroberson.net> >>>>> wrote: >>>>> >>>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>>> >>>>> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>>>> >>>>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>>>> >>>>> The following trivial patch fixes the issue on my W3520 processor; >>>>> >>>>> AFAICS >>>>> >>>>> it's what should be done after reading several of the specs because the >>>>> >>>>> logical count that's tracked with ebx is exactly what is needed for >>>>> >>>>> logical_cpus (it's an absolute quantity). I need to verify it with a >>>>> >>>>> multi-cpu >>>>> >>>>> topology at work (the two r710s I was testing with E-series Xeons on >>>>> >>>>> aren't >>>>> >>>>> available remotely right now). >>>>> >>>>> Thanks! >>>>> >>>>> -Garrett >>>>> >>>>> Jung-uk Kim and Attilio Rao have both been looking at this code recently >>>>> >>>>> and >>>>> >>>>> are in a better position to review the patch in the PR. >>>>> >>>>> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >>>>> >>>>> The patch works as expected (it now properly detects the SMIT CPUs as >>>>> >>>>> logical CPUs), but setting machdep.hlt_logical_cpus=1 causes other >>>>> >>>>> problems with scheduling tasks because certain kernel threads get >>>>> >>>>> stuck at boot when netbooting (in particular I've seen problems with >>>>> >>>>> usbhub* and a few others bits), so in order for >>>>> >>>>> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >>>>> >>>>> require some changes to the ULE scheduler to shuffle around the >>>>> >>>>> threads to available cores/processors? >>>>> >>>>> >>>>> hlt_logical_cpus should be rewritten to use cpusets to change the >>>>> default >>>>> >>>>> system set rather than specifically halting those cpus. There are a >>>>> number >>>>> >>>>> of loops in the kernel that iterate over all cpus and attempt to bind >>>>> and >>>>> >>>>> perform some task. I think there are a number of other reasons to >>>>> prefer a >>>>> >>>>> less aggressive approach to avoiding the logical cpus as well. Simply >>>>> >>>>> preventing user thread schedule will achieve the intent of the sysctl in >>>>> any >>>>> >>>>> event. >>>>> >>>>> Ok... in that event then the bug is ok, but maybe I should add >>>>> >>>>> some code to the patch to warn the user about functional issues >>>>> >>>>> associated with halting logical CPUs? >>>>> >>>>> I don't think the bug is ok. We probably shouldn't have sysctls which >>>>> readily break the kernel. As I said we should instead have the sysctl >>>>> backend to cpuset. It shouldn't take more than an hour to code and >>>>> test. >>>> >>>> Ok.. I'll look at this once I have my other system back online so >>>> I can actively break something until I get it to work. >>> >>> BTW... there's a lot of code in machdep.c that does the same thing >>> to idle the CPU, for instance, cpu_idle_hlt, cpu_idle_acpi, >>> cpu_idle_amdc1e (on amd64). What should be done about those cases >>> (same thing, or different)? >> >> Those are the actual idle functions that the scheduler uses. Those are >> safe. > > I'll look into running this on a Nehalem processor machine, but > this appears to as expected on my Penryn processor test machine with > machdep.hlt_cpus = { 110, 101, 11, 0 } and with machdep.idle=acpi; I'm > not sure if the if the loop is supposed to be there still, but it > wouldn't make sense because the CPU would be spinning in the kernel. Sorry.. forgot the patch :(. -Garrett On Wed, 25 Aug 2010, Garrett Cooper wrote: > On Tue, Aug 24, 2010 at 9:53 PM, Jeff Roberson <jroberson@jroberson.net> wrote: >> On Tue, 24 Aug 2010, Garrett Cooper wrote: >> >>> On Tue, Aug 24, 2010 at 3:45 PM, Garrett Cooper <yaneurabeya@gmail.com> >>> wrote: >>>> >>>> On Tue, Aug 24, 2010 at 2:51 PM, Garrett Cooper <yaneurabeya@gmail.com> >>>> wrote: >>>>> >>>>> On Aug 24, 2010, at 2:03 PM, Jeff Roberson wrote: >>>>> >>>>> >>>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>>> >>>>> On Tue, Aug 24, 2010 at 12:22 PM, Jeff Roberson >>>>> <jroberson@jroberson.net> >>>>> wrote: >>>>> >>>>> On Tue, 24 Aug 2010, Garrett Cooper wrote: >>>>> >>>>> On Mon, Aug 23, 2010 at 6:33 AM, John Baldwin <jhb@freebsd.org> wrote: >>>>> >>>>> On Sunday, August 22, 2010 4:17:37 am Garrett Cooper wrote: >>>>> >>>>> The following trivial patch fixes the issue on my W3520 processor; >>>>> >>>>> AFAICS >>>>> >>>>> it's what should be done after reading several of the specs because the >>>>> >>>>> logical count that's tracked with ebx is exactly what is needed for >>>>> >>>>> logical_cpus (it's an absolute quantity). I need to verify it with a >>>>> >>>>> multi-cpu >>>>> >>>>> topology at work (the two r710s I was testing with E-series Xeons on >>>>> >>>>> aren't >>>>> >>>>> available remotely right now). >>>>> >>>>> Thanks! >>>>> >>>>> -Garrett >>>>> >>>>> Jung-uk Kim and Attilio Rao have both been looking at this code recently >>>>> >>>>> and >>>>> >>>>> are in a better position to review the patch in the PR. >>>>> >>>>> (Moving jhb@ to BCC, adding jeff@ for possible input on ULE) >>>>> >>>>> The patch works as expected (it now properly detects the SMIT CPUs as >>>>> >>>>> logical CPUs), but setting machdep.hlt_logical_cpus=1 causes other >>>>> >>>>> problems with scheduling tasks because certain kernel threads get >>>>> >>>>> stuck at boot when netbooting (in particular I've seen problems with >>>>> >>>>> usbhub* and a few others bits), so in order for >>>>> >>>>> machdep.hlt_logical_cpus to be fixed on SMT processors, it might >>>>> >>>>> require some changes to the ULE scheduler to shuffle around the >>>>> >>>>> threads to available cores/processors? >>>>> >>>>> >>>>> hlt_logical_cpus should be rewritten to use cpusets to change the >>>>> default >>>>> >>>>> system set rather than specifically halting those cpus. There are a >>>>> number >>>>> >>>>> of loops in the kernel that iterate over all cpus and attempt to bind >>>>> and >>>>> >>>>> perform some task. I think there are a number of other reasons to >>>>> prefer a >>>>> >>>>> less aggressive approach to avoiding the logical cpus as well. Simply >>>>> >>>>> preventing user thread schedule will achieve the intent of the sysctl in >>>>> any >>>>> >>>>> event. >>>>> >>>>> Ok... in that event then the bug is ok, but maybe I should add >>>>> >>>>> some code to the patch to warn the user about functional issues >>>>> >>>>> associated with halting logical CPUs? >>>>> >>>>> I don't think the bug is ok. We probably shouldn't have sysctls which >>>>> readily break the kernel. As I said we should instead have the sysctl >>>>> backend to cpuset. It shouldn't take more than an hour to code and >>>>> test. >>>> >>>> Ok.. I'll look at this once I have my other system back online so >>>> I can actively break something until I get it to work. >>> >>> BTW... there's a lot of code in machdep.c that does the same thing >>> to idle the CPU, for instance, cpu_idle_hlt, cpu_idle_acpi, >>> cpu_idle_amdc1e (on amd64). What should be done about those cases >>> (same thing, or different)? >> >> Those are the actual idle functions that the scheduler uses. Those are >> safe. > > I'll look into running this on a Nehalem processor machine, but > this appears to as expected on my Penryn processor test machine with > machdep.hlt_cpus = { 110, 101, 11, 0 } and with machdep.idle=acpi; I'm > not sure if the if the loop is supposed to be there still, but it > wouldn't make sense because the CPU would be spinning in the kernel. This doesn't actually idle the cores. You need to change the root cpuset to remove cpus. Jeff > Thanks, > -Garrett > Regarding the part of the patch that sets logical_cpus - I don't really understand what logical_cpus represents. It seems that in topo_probe_0x4() it's set to maximum (possible) number of logical CPUs per whole package (for BSP package). In topo_probe_0xb(), as you've spotted, it was not set at all. We already have cpu_logical variable (talk about confusing names) and that represents number of logical CPUs per core. So, I think that logical_cpus should be abolished altogether and cpu_logical should be used in its place. That would be the end of topo_probe_0x4() where hyperthreading_cpus is assigned with fallback value and init_secondary() where it's used to set logical_cpus_mask. P.S. The code also doesn't seem to properly handle a case where boot_cpu_id % logical_cpus != 0 and/or boot_cpu_id % hyperthreading_cpus !=0 Not sure if that ever happens in real systems, though. -- Andriy Gapon Author: avg Date: Fri Oct 1 10:32:54 2010 New Revision: 213323 URL: http://svn.freebsd.org/changeset/base/213323 Log: i386 and amd64 mp_machdep: improve topology detection for Intel CPUs This patch is significantly based on previous work by jkim. List of changes: - added comments that describe topology uniformity assumption - added reference to Intel Processor Topology Enumeration article - documented a few global variables that describe topology - retired weirdly set and used logical_cpus variable - changed fallback code for mp_ncpus > 0 case, so that CPUs are treated as being different packages rather than cores in a single package - moved AMD-specific code to topo_probe_amd [jkim] - in topo_probe_0x4() follow Intel-prescribed procedure of deriving SMT and core masks and match APIC IDs against those masks [started by jkim] - in topo_probe_0x4() drop code for double-checking topology parameters by looking at L1 cache properties [jkim] - in topo_probe_0xb() add fallback path to topo_probe_0x4() as prescribed by Intel [jkim] Still to do: - prepare for upcoming AMD CPUs by using new mechanism of uniform topology description [pointed by jkim] - probe cache topology in addition to CPU topology and probably use that for scheduler affinity topology; e.g. Core2 Duo and Athlon II X2 have the same CPU topology, but Athlon cores do not share L2 cache while Core2's do (no L3 cache in both cases) - think of supporting non-uniform topologies if they are ever implemented for platforms in question - think how to better described old HTT vs new HTT distinction, HTT vs SMT can be confusing as SMT is a generic term - more robust code for marking CPUs as "logical" and/or "hyperthreaded", use HTT mask instead of modulo operation - correct support for halting logical and/or hyperthreaded CPUs, let scheduler know that it shouldn't schedule any threads on those CPUs PR: kern/145385 (related) In collaboration with: jkim Tested by: Sergey Kandaurov <pluknet@gmail.com>, Jeremy Chadwick <freebsd@jdc.parodius.com>, Chip Camden <sterling@camdensoftware.com>, Steve Wills <steve@mouf.net>, Olivier Smedts <olivier@gid0.org>, Florian Smeets <flo@smeets.im> MFC after: 1 month Modified: head/sys/amd64/amd64/mp_machdep.c head/sys/i386/i386/mp_machdep.c Modified: head/sys/amd64/amd64/mp_machdep.c ============================================================================== --- head/sys/amd64/amd64/mp_machdep.c Fri Oct 1 09:34:41 2010 (r213322) +++ head/sys/amd64/amd64/mp_machdep.c Fri Oct 1 10:32:54 2010 (r213323) @@ -126,7 +126,6 @@ extern inthand_t IDTVEC(fast_syscall), I * Local data and functions. */ -static u_int logical_cpus; static volatile cpumask_t ipi_nmi_pending; /* used to hold the AP's until we are ready to release them */ @@ -152,8 +151,8 @@ int apic_cpuids[MAX_APIC_ID + 1]; static volatile u_int cpu_ipi_pending[MAXCPU]; static u_int boot_address; -static int cpu_logical; -static int cpu_cores; +static int cpu_logical; /* logical cpus per core */ +static int cpu_cores; /* cores per package */ static void assign_cpu_ids(void); static void set_interrupt_apic_ids(void); @@ -162,7 +161,7 @@ static int start_ap(int apic_id); static void release_aps(void *dummy); static int hlt_logical_cpus; -static u_int hyperthreading_cpus; +static u_int hyperthreading_cpus; /* logical cpus sharing L1 cache */ static cpumask_t hyperthreading_cpus_mask; static int hyperthreading_allowed = 1; static struct sysctl_ctx_list logical_cpu_clist; @@ -176,24 +175,105 @@ mem_range_AP_init(void) } static void +topo_probe_amd(void) +{ + + /* AMD processors do not support HTT. */ + cpu_cores = (amd_feature2 & AMDID2_CMP) != 0 ? + (cpu_procinfo2 & AMDID_CMP_CORES) + 1 : 1; + cpu_logical = 1; +} + +/* + * Round up to the next power of two, if necessary, and then + * take log2. + * Returns -1 if argument is zero. + */ +static __inline int +mask_width(u_int x) +{ + + return (fls(x << (1 - powerof2(x))) - 1); +} + +static void +topo_probe_0x4(void) +{ + u_int p[4]; + int pkg_id_bits; + int core_id_bits; + int max_cores; + int max_logical; + int id; + + /* Both zero and one here mean one logical processor per package. */ + max_logical = (cpu_feature & CPUID_HTT) != 0 ? + (cpu_procinfo & CPUID_HTT_CORES) >> 16 : 1; + if (max_logical <= 1) + return; + + /* + * Because of uniformity assumption we examine only + * those logical processors that belong to the same + * package as BSP. Further, we count number of + * logical processors that belong to the same core + * as BSP thus deducing number of threads per core. + */ + cpuid_count(0x04, 0, p); + max_cores = ((p[0] >> 26) & 0x3f) + 1; + core_id_bits = mask_width(max_logical/max_cores); + if (core_id_bits < 0) + return; + pkg_id_bits = core_id_bits + mask_width(max_cores); + + for (id = 0; id <= MAX_APIC_ID; id++) { + /* Check logical CPU availability. */ + if (!cpu_info[id].cpu_present || cpu_info[id].cpu_disabled) + continue; + /* Check if logical CPU has the same package ID. */ + if ((id >> pkg_id_bits) != (boot_cpu_id >> pkg_id_bits)) + continue; + cpu_cores++; + /* Check if logical CPU has the same package and core IDs. */ + if ((id >> core_id_bits) == (boot_cpu_id >> core_id_bits)) + cpu_logical++; + } + + cpu_cores /= cpu_logical; + hyperthreading_cpus = cpu_logical; +} + +static void topo_probe_0xb(void) { - int logical; - int p[4]; + u_int p[4]; int bits; - int type; int cnt; int i; + int logical; + int type; int x; - /* We only support two levels for now. */ + /* We only support three levels for now. */ for (i = 0; i < 3; i++) { - cpuid_count(0x0B, i, p); + cpuid_count(0x0b, i, p); + + /* Fall back if CPU leaf 11 doesn't really exist. */ + if (i == 0 && p[1] == 0) { + topo_probe_0x4(); + return; + } + bits = p[0] & 0x1f; logical = p[1] &= 0xffff; type = (p[2] >> 8) & 0xff; if (type == 0 || logical == 0) break; + /* + * Because of uniformity assumption we examine only + * those logical processors that belong to the same + * package as BSP. + */ for (cnt = 0, x = 0; x <= MAX_APIC_ID; x++) { if (!cpu_info[x].cpu_present || cpu_info[x].cpu_disabled) @@ -211,76 +291,16 @@ topo_probe_0xb(void) cpu_cores /= cpu_logical; } -static void -topo_probe_0x4(void) -{ - u_int threads_per_cache, p[4]; - u_int htt, cmp; - int i; - - htt = cmp = 1; - /* - * If this CPU supports HTT or CMP then mention the - * number of physical/logical cores it contains. - */ - if (cpu_feature & CPUID_HTT) - htt = (cpu_procinfo & CPUID_HTT_CORES) >> 16; - if (cpu_vendor_id == CPU_VENDOR_AMD && (amd_feature2 & AMDID2_CMP)) - cmp = (cpu_procinfo2 & AMDID_CMP_CORES) + 1; - else if (cpu_vendor_id == CPU_VENDOR_INTEL && (cpu_high >= 4)) { - cpuid_count(4, 0, p); - if ((p[0] & 0x1f) != 0) - cmp = ((p[0] >> 26) & 0x3f) + 1; - } - cpu_cores = cmp; - cpu_logical = htt / cmp; - - /* Setup the initial logical CPUs info. */ - if (cpu_feature & CPUID_HTT) - logical_cpus = (cpu_procinfo & CPUID_HTT_CORES) >> 16; - - /* - * Work out if hyperthreading is *really* enabled. This - * is made really ugly by the fact that processors lie: Dual - * core processors claim to be hyperthreaded even when they're - * not, presumably because they want to be treated the same - * way as HTT with respect to per-cpu software licensing. - * At the time of writing (May 12, 2005) the only hyperthreaded - * cpus are from Intel, and Intel's dual-core processors can be - * identified via the "deterministic cache parameters" cpuid - * calls. - */ - /* - * First determine if this is an Intel processor which claims - * to have hyperthreading support. - */ - if ((cpu_feature & CPUID_HTT) && cpu_vendor_id == CPU_VENDOR_INTEL) { - /* - * If the "deterministic cache parameters" cpuid calls - * are available, use them. - */ - if (cpu_high >= 4) { - /* Ask the processor about the L1 cache. */ - for (i = 0; i < 1; i++) { - cpuid_count(4, i, p); - threads_per_cache = ((p[0] & 0x3ffc000) >> 14) + 1; - if (hyperthreading_cpus < threads_per_cache) - hyperthreading_cpus = threads_per_cache; - if ((p[0] & 0x1f) == 0) - break; - } - } - - /* - * If the deterministic cache parameters are not - * available, or if no caches were reported to exist, - * just accept what the HTT flag indicated. - */ - if (hyperthreading_cpus == 0) - hyperthreading_cpus = logical_cpus; - } -} - +/* + * Both topology discovery code and code that consumes topology + * information assume top-down uniformity of the topology. + * That is, all physical packages must be identical and each + * core in a package must have the same number of threads. + * Topology information is queried only on BSP, on which this + * code runs and for which it can query CPUID information. + * Then topology is extrapolated on all packages using the + * uniformity assumption. + */ static void topo_probe(void) { @@ -289,13 +309,31 @@ topo_probe(void) if (cpu_topo_probed) return; - logical_cpus = logical_cpus_mask = 0; - if (cpu_high >= 0xb) - topo_probe_0xb(); - else if (cpu_high) - topo_probe_0x4(); + logical_cpus_mask = 0; + if (cpu_vendor_id == CPU_VENDOR_AMD) + topo_probe_amd(); + else if (cpu_vendor_id == CPU_VENDOR_INTEL) { + /* + * See Intel(R) 64 Architecture Processor + * Topology Enumeration article for details. + * + * Note that 0x1 <= cpu_high < 4 case should be + * compatible with topo_probe_0x4() logic when + * CPUID.1:EBX[23:16] > 0 (cpu_cores will be 1) + * or it should trigger the fallback otherwise. + */ + if (cpu_high >= 0xb) + topo_probe_0xb(); + else if (cpu_high >= 0x1) + topo_probe_0x4(); + } + + /* + * Fallback: assume each logical CPU is in separate + * physical package. That is, no multi-core, no SMT. + */ if (cpu_cores == 0) - cpu_cores = mp_ncpus > 0 ? mp_ncpus : 1; + cpu_cores = 1; if (cpu_logical == 0) cpu_logical = 1; cpu_topo_probed = 1; @@ -675,7 +713,8 @@ init_secondary(void) printf("SMP: AP CPU #%d Launched!\n", PCPU_GET(cpuid)); /* Determine if we are a logical CPU. */ - if (logical_cpus > 1 && PCPU_GET(apic_id) % logical_cpus != 0) + /* XXX Calculation depends on cpu_logical being a power of 2, e.g. 2 */ + if (cpu_logical > 1 && PCPU_GET(apic_id) % cpu_logical != 0) logical_cpus_mask |= PCPU_GET(cpumask); /* Determine if we are a hyperthread. */ Modified: head/sys/i386/i386/mp_machdep.c ============================================================================== --- head/sys/i386/i386/mp_machdep.c Fri Oct 1 09:34:41 2010 (r213322) +++ head/sys/i386/i386/mp_machdep.c Fri Oct 1 10:32:54 2010 (r213323) @@ -173,7 +173,6 @@ static u_long *ipi_hardclock_counts[MAXC * Local data and functions. */ -static u_int logical_cpus; static volatile cpumask_t ipi_nmi_pending; /* used to hold the AP's until we are ready to release them */ @@ -199,8 +198,8 @@ int apic_cpuids[MAX_APIC_ID + 1]; static volatile u_int cpu_ipi_pending[MAXCPU]; static u_int boot_address; -static int cpu_logical; -static int cpu_cores; +static int cpu_logical; /* logical cpus per core */ +static int cpu_cores; /* cores per package */ static void assign_cpu_ids(void); static void install_ap_tramp(void); @@ -210,7 +209,7 @@ static int start_ap(int apic_id); static void release_aps(void *dummy); static int hlt_logical_cpus; -static u_int hyperthreading_cpus; +static u_int hyperthreading_cpus; /* logical cpus sharing L1 cache */ static cpumask_t hyperthreading_cpus_mask; static int hyperthreading_allowed = 1; static struct sysctl_ctx_list logical_cpu_clist; @@ -223,24 +222,105 @@ mem_range_AP_init(void) } static void +topo_probe_amd(void) +{ + + /* AMD processors do not support HTT. */ + cpu_cores = (amd_feature2 & AMDID2_CMP) != 0 ? + (cpu_procinfo2 & AMDID_CMP_CORES) + 1 : 1; + cpu_logical = 1; +} + +/* + * Round up to the next power of two, if necessary, and then + * take log2. + * Returns -1 if argument is zero. + */ +static __inline int +mask_width(u_int x) +{ + + return (fls(x << (1 - powerof2(x))) - 1); +} + +static void +topo_probe_0x4(void) +{ + u_int p[4]; + int pkg_id_bits; + int core_id_bits; + int max_cores; + int max_logical; + int id; + + /* Both zero and one here mean one logical processor per package. */ + max_logical = (cpu_feature & CPUID_HTT) != 0 ? + (cpu_procinfo & CPUID_HTT_CORES) >> 16 : 1; + if (max_logical <= 1) + return; + + /* + * Because of uniformity assumption we examine only + * those logical processors that belong to the same + * package as BSP. Further, we count number of + * logical processors that belong to the same core + * as BSP thus deducing number of threads per core. + */ + cpuid_count(0x04, 0, p); + max_cores = ((p[0] >> 26) & 0x3f) + 1; + core_id_bits = mask_width(max_logical/max_cores); + if (core_id_bits < 0) + return; + pkg_id_bits = core_id_bits + mask_width(max_cores); + + for (id = 0; id <= MAX_APIC_ID; id++) { + /* Check logical CPU availability. */ + if (!cpu_info[id].cpu_present || cpu_info[id].cpu_disabled) + continue; + /* Check if logical CPU has the same package ID. */ + if ((id >> pkg_id_bits) != (boot_cpu_id >> pkg_id_bits)) + continue; + cpu_cores++; + /* Check if logical CPU has the same package and core IDs. */ + if ((id >> core_id_bits) == (boot_cpu_id >> core_id_bits)) + cpu_logical++; + } + + cpu_cores /= cpu_logical; + hyperthreading_cpus = cpu_logical; +} + +static void topo_probe_0xb(void) { - int logical; - int p[4]; + u_int p[4]; int bits; - int type; int cnt; int i; + int logical; + int type; int x; - /* We only support two levels for now. */ + /* We only support three levels for now. */ for (i = 0; i < 3; i++) { - cpuid_count(0x0B, i, p); + cpuid_count(0x0b, i, p); + + /* Fall back if CPU leaf 11 doesn't really exist. */ + if (i == 0 && p[1] == 0) { + topo_probe_0x4(); + return; + } + bits = p[0] & 0x1f; logical = p[1] &= 0xffff; type = (p[2] >> 8) & 0xff; if (type == 0 || logical == 0) break; + /* + * Because of uniformity assumption we examine only + * those logical processors that belong to the same + * package as BSP. + */ for (cnt = 0, x = 0; x <= MAX_APIC_ID; x++) { if (!cpu_info[x].cpu_present || cpu_info[x].cpu_disabled) @@ -258,76 +338,16 @@ topo_probe_0xb(void) cpu_cores /= cpu_logical; } -static void -topo_probe_0x4(void) -{ - u_int threads_per_cache, p[4]; - u_int htt, cmp; - int i; - - htt = cmp = 1; - /* - * If this CPU supports HTT or CMP then mention the - * number of physical/logical cores it contains. - */ - if (cpu_feature & CPUID_HTT) - htt = (cpu_procinfo & CPUID_HTT_CORES) >> 16; - if (cpu_vendor_id == CPU_VENDOR_AMD && (amd_feature2 & AMDID2_CMP)) - cmp = (cpu_procinfo2 & AMDID_CMP_CORES) + 1; - else if (cpu_vendor_id == CPU_VENDOR_INTEL && (cpu_high >= 4)) { - cpuid_count(4, 0, p); - if ((p[0] & 0x1f) != 0) - cmp = ((p[0] >> 26) & 0x3f) + 1; - } - cpu_cores = cmp; - cpu_logical = htt / cmp; - - /* Setup the initial logical CPUs info. */ - if (cpu_feature & CPUID_HTT) - logical_cpus = (cpu_procinfo & CPUID_HTT_CORES) >> 16; - - /* - * Work out if hyperthreading is *really* enabled. This - * is made really ugly by the fact that processors lie: Dual - * core processors claim to be hyperthreaded even when they're - * not, presumably because they want to be treated the same - * way as HTT with respect to per-cpu software licensing. - * At the time of writing (May 12, 2005) the only hyperthreaded - * cpus are from Intel, and Intel's dual-core processors can be - * identified via the "deterministic cache parameters" cpuid - * calls. - */ - /* - * First determine if this is an Intel processor which claims - * to have hyperthreading support. - */ - if ((cpu_feature & CPUID_HTT) && cpu_vendor_id == CPU_VENDOR_INTEL) { - /* - * If the "deterministic cache parameters" cpuid calls - * are available, use them. - */ - if (cpu_high >= 4) { - /* Ask the processor about the L1 cache. */ - for (i = 0; i < 1; i++) { - cpuid_count(4, i, p); - threads_per_cache = ((p[0] & 0x3ffc000) >> 14) + 1; - if (hyperthreading_cpus < threads_per_cache) - hyperthreading_cpus = threads_per_cache; - if ((p[0] & 0x1f) == 0) - break; - } - } - - /* - * If the deterministic cache parameters are not - * available, or if no caches were reported to exist, - * just accept what the HTT flag indicated. - */ - if (hyperthreading_cpus == 0) - hyperthreading_cpus = logical_cpus; - } -} - +/* + * Both topology discovery code and code that consumes topology + * information assume top-down uniformity of the topology. + * That is, all physical packages must be identical and each + * core in a package must have the same number of threads. + * Topology information is queried only on BSP, on which this + * code runs and for which it can query CPUID information. + * Then topology is extrapolated on all packages using the + * uniformity assumption. + */ static void topo_probe(void) { @@ -336,13 +356,31 @@ topo_probe(void) if (cpu_topo_probed) return; - logical_cpus = logical_cpus_mask = 0; - if (cpu_high >= 0xb) - topo_probe_0xb(); - else if (cpu_high) - topo_probe_0x4(); + logical_cpus_mask = 0; + if (cpu_vendor_id == CPU_VENDOR_AMD) + topo_probe_amd(); + else if (cpu_vendor_id == CPU_VENDOR_INTEL) { + /* + * See Intel(R) 64 Architecture Processor + * Topology Enumeration article for details. + * + * Note that 0x1 <= cpu_high < 4 case should be + * compatible with topo_probe_0x4() logic when + * CPUID.1:EBX[23:16] > 0 (cpu_cores will be 1) + * or it should trigger the fallback otherwise. + */ + if (cpu_high >= 0xb) + topo_probe_0xb(); + else if (cpu_high >= 0x1) + topo_probe_0x4(); + } + + /* + * Fallback: assume each logical CPU is in separate + * physical package. That is, no multi-core, no SMT. + */ if (cpu_cores == 0) - cpu_cores = mp_ncpus > 0 ? mp_ncpus : 1; + cpu_cores = 1; if (cpu_logical == 0) cpu_logical = 1; cpu_topo_probed = 1; @@ -706,7 +744,8 @@ init_secondary(void) printf("SMP: AP CPU #%d Launched!\n", PCPU_GET(cpuid)); /* Determine if we are a logical CPU. */ - if (logical_cpus > 1 && PCPU_GET(apic_id) % logical_cpus != 0) + /* XXX Calculation depends on cpu_logical being a power of 2, e.g. 2 */ + if (cpu_logical > 1 && PCPU_GET(apic_id) % cpu_logical != 0) logical_cpus_mask |= PCPU_GET(cpumask); /* Determine if we are a hyperthread. */ _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" Here's a patch to remove halted logical processors from root cpu set (cpuset number zero) and consequently all child sets: http://people.freebsd.org/~avg/cpu-halt.diff Please note that unhalting CPUs will add them to cpuset zero, but will not (re-)add them cpusets of the running processes. So additional action will be required from system administrator if e would like existing processes to make use of unhalted CPUs. Also, interrupts that are bound to halted CPUs are not rebound on halt, and so will be delivered to halted CPUs. This should not be a problem for typical environments, but would be nice to fix anyway. -- Andriy Gapon On Thu, Oct 7, 2010 at 10:03 AM, Andriy Gapon <avg@freebsd.org> wrote: > > Here's a patch to remove halted logical processors from root cpu set (cpu= set > number zero) and consequently all child sets: > http://people.freebsd.org/~avg/cpu-halt.diff > > Please note that unhalting CPUs will add them to cpuset zero, but will no= t > (re-)add them cpusets of the running processes. =A0So additional action w= ill be > required from system administrator if e would like existing processes to = make use > of unhalted CPUs. > > Also, interrupts that are bound to halted CPUs are not rebound on halt, a= nd so > will be delivered to halted CPUs. =A0This should not be a problem for typ= ical > environments, but would be nice to fix anyway. Sorry for taking so long to get back on this item. The patch looks ok as far as setting the CPUSET is concerned (setting machdep.hlt_logical_cpus=3D1 works on an r710 with 8 logical procs as SCHED_ULE schedules all of the tasks on the non-logical SMT cores, not the logical ones, and the patch properly detects that there aren't any logical processors on a Core2 Quad I have at home), but it's missing a key case where I go from... machdep.hlt_logical_cpus: 1 -> 0 ... it should reset CPUSETZERO. The overall CPU topology should probably be cached and restored when reset, or at least CPUSETZERO should be reset (probably the simpler and cleaner route to go). After that all that's required is work for i386 and I'd vote for a commit of the patch. Thanks for the hard work Andriy :)! -Garrett On Wed, Nov 17, 2010 at 4:34 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote= : > On Thu, Oct 7, 2010 at 10:03 AM, Andriy Gapon <avg@freebsd.org> wrote: >> >> Here's a patch to remove halted logical processors from root cpu set (cp= uset >> number zero) and consequently all child sets: >> http://people.freebsd.org/~avg/cpu-halt.diff >> >> Please note that unhalting CPUs will add them to cpuset zero, but will n= ot >> (re-)add them cpusets of the running processes. =A0So additional action = will be >> required from system administrator if e would like existing processes to= make use >> of unhalted CPUs. >> >> Also, interrupts that are bound to halted CPUs are not rebound on halt, = and so >> will be delivered to halted CPUs. =A0This should not be a problem for ty= pical >> environments, but would be nice to fix anyway. > > =A0 =A0Sorry for taking so long to get back on this item. The patch looks > ok as far as setting the CPUSET is concerned (setting > machdep.hlt_logical_cpus=3D1 works on an r710 with 8 logical procs as > SCHED_ULE schedules all of the tasks on the non-logical SMT cores, not > the logical ones, and the patch properly detects that there aren't any > logical processors on a Core2 Quad I have at home), but it's missing a > key case where I go from... > > machdep.hlt_logical_cpus: 1 -> 0 > > =A0 =A0... it should reset CPUSETZERO. > =A0 =A0The overall CPU topology should probably be cached and restored > when reset, or at least CPUSETZERO should be reset (probably the > simpler and cleaner route to go). > =A0 =A0After that all that's required is work for i386 and I'd vote for a > commit of the patch. > Thanks for the hard work Andriy :)! Just to illustrate, here's how I reproduced the successful behavior (and the problem): 1. Execute the following script: #!/bin/sh i=3D0 ncpus=3D`sysctl -n kern.smp.cpus` while [ $i -lt $ncpus ] ; do while : ; do : ; done & : $(( i +=3D 1 )) done 2. Execute sysctl machdep.hlt_logical_cpus=3D1 . 3. Look at cpuset info; example: Before: # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` ; do cpuset -g -p $i ; done pid 2179 mask: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 pid 2180 mask: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 After: # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` ; do cpuset -g -p $i ; done pid 2179 mask: 0, 2, 4, 6, 8, 10, 12, 14 pid 2180 mask: 0, 2, 4, 6, 8, 10, 12, 14 # sysctl machdep.hlt_logical_cpus=3D0 machdep.hlt_logical_cpus: 1 -> 0 # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` ; do cpuset -g -p $i ; done pid 2179 mask: 0, 2, 4, 6, 8, 10, 12, 14 pid 2180 mask: 0, 2, 4, 6, 8, 10, 12, 14 I'm taking the patch and running with it now to try and get it working 100%= . On Thu, Nov 18, 2010 at 4:09 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: > On Wed, Nov 17, 2010 at 4:34 PM, Garrett Cooper <yaneurabeya@gmail.com> wrote: >> On Thu, Oct 7, 2010 at 10:03 AM, Andriy Gapon <avg@freebsd.org> wrote: >>> >>> Here's a patch to remove halted logical processors from root cpu set (cpuset >>> number zero) and consequently all child sets: >>> http://people.freebsd.org/~avg/cpu-halt.diff >>> >>> Please note that unhalting CPUs will add them to cpuset zero, but will not >>> (re-)add them cpusets of the running processes. So additional action will be >>> required from system administrator if e would like existing processes to make use >>> of unhalted CPUs. >>> >>> Also, interrupts that are bound to halted CPUs are not rebound on halt, and so >>> will be delivered to halted CPUs. This should not be a problem for typical >>> environments, but would be nice to fix anyway. >> >> Sorry for taking so long to get back on this item. The patch looks >> ok as far as setting the CPUSET is concerned (setting >> machdep.hlt_logical_cpus=1 works on an r710 with 8 logical procs as >> SCHED_ULE schedules all of the tasks on the non-logical SMT cores, not >> the logical ones, and the patch properly detects that there aren't any >> logical processors on a Core2 Quad I have at home), but it's missing a >> key case where I go from... >> >> machdep.hlt_logical_cpus: 1 -> 0 >> >> ... it should reset CPUSETZERO. >> The overall CPU topology should probably be cached and restored >> when reset, or at least CPUSETZERO should be reset (probably the >> simpler and cleaner route to go). >> After that all that's required is work for i386 and I'd vote for a >> commit of the patch. >> Thanks for the hard work Andriy :)! > > Just to illustrate, here's how I reproduced the successful behavior > (and the problem): > > 1. Execute the following script: > > #!/bin/sh > i=0 > ncpus=`sysctl -n kern.smp.cpus` > while [ $i -lt $ncpus ] ; do > while : ; do : ; done & > : $(( i += 1 )) > done > > 2. Execute sysctl machdep.hlt_logical_cpus=1 . > 3. Look at cpuset info; example: > > Before: > > # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` > ; do cpuset -g -p $i ; done > pid 2179 mask: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 > pid 2180 mask: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 > > After: > > # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` > ; do cpuset -g -p $i ; done > pid 2179 mask: 0, 2, 4, 6, 8, 10, 12, 14 > pid 2180 mask: 0, 2, 4, 6, 8, 10, 12, 14 > # sysctl machdep.hlt_logical_cpus=0 > machdep.hlt_logical_cpus: 1 -> 0 > # for i in `ps aux | grep gcooper | grep -v grep | awk '{ print $2 }'` > ; do cpuset -g -p $i ; done > pid 2179 mask: 0, 2, 4, 6, 8, 10, 12, 14 > pid 2180 mask: 0, 2, 4, 6, 8, 10, 12, 14 > > I'm taking the patch and running with it now to try and get it working 100%. This output was from a patch in progress based on Andriy's work, that seems to get the bits in place properly: test-26# sysctl machdep.hlt_logical_cpus=1 sysctl_hlt_logical_cpus: mask: 43690 cpuset_zero_modify (before): cpuset_zero: 65535, mask: 21845 cpuset_update (after): set: 21845, mask: 21845 cpuset_update (after): set: 21845, mask: 21845 cpuset_zero_modify (after): cpuset_zero: 21845, mask: 21845 machdep.hlt_logical_cpus: 0 -> 1 test-26# sysctl machdep.hlt_logical_cpus=0 sysctl_hlt_logical_cpus: mask: 0 cpuset_zero_modify (before): cpuset_zero: 21845, mask: 65535 cpuset_update (after): set: 21845, mask: 65535 cpuset_update (after): set: 21845, mask: 21845 cpuset_zero_modify (after): cpuset_zero: 65535, mask: 65535 machdep.hlt_logical_cpus: 1 -> 0 test-26# Weird thing is that the CPU affinity isn't adjusted even though cpuset_zero appears to be modified. I think this is the problem: static int cpuset_testupdate(struct cpuset *set, cpuset_t *mask) { struct cpuset *nset; cpuset_t newmask; int error; mtx_assert(&cpuset_lock, MA_OWNED); if (set->cs_flags & CPU_SET_RDONLY) return (EPERM); if (!CPU_OVERLAP(&set->cs_mask, mask)) return (EDEADLK); CPU_COPY(&set->cs_mask, &newmask); CPU_AND(&newmask, mask); error = 0; LIST_FOREACH(nset, &set->cs_children, cs_siblings) if ((error = cpuset_testupdate(nset, &newmask)) != 0) break; return (error); } ... /* * Applies the mask 'mask' without checking for empty sets or permissions. */ static void cpuset_update(struct cpuset *set, cpuset_t *mask) { struct cpuset *nset; mtx_assert(&cpuset_lock, MA_OWNED); CPU_AND(&set->cs_mask, mask); LIST_FOREACH(nset, &set->cs_children, cs_siblings) cpuset_update(nset, &set->cs_mask); return; } Note that it's doing CPU_AND, not CPU_OR, in both routines that get called from cpuset_modify. This makes sense to reject bad input (because scheduling on non-existent CPUs is a bad thing :/), but sucks in our case because it means that we can't use cpuset_modify [easily]... Thanks! -Garrett I think that I have explained the behavior that you see and other problems with the patch in the email in which I submitted the patch. -- Andriy Gapon Author: avg Date: Wed Jun 8 08:12:15 2011 New Revision: 222853 URL: http://svn.freebsd.org/changeset/base/222853 Log: remove code for dynamic offlining/onlining of CPUs on x86 The code has definitely been broken for SCHED_ULE, which is a default scheduler. It may have been broken for SCHED_4BSD in more subtle ways, e.g. with manually configured CPU affinities and for interrupt devilery purposes. We still provide a way to disable individual CPUs or all hyperthreading "twin" CPUs before SMP startup. See the UPDATING entry for details. Interaction between building CPU topology and disabling CPUs still remains fuzzy: topology is first built using all availble CPUs and then the disabled CPUs should be "subtracted" from it. That doesn't work well if the resulting topology becomes non-uniform. This work is done in cooperation with Attilio Rao who in addition to reviewing also provided parts of code. PR: kern/145385 Discussed with: gcooper, ambrisko, mdf, sbruno Reviewed by: attilio Tested by: pho, pluknet X-MFC after: never Modified: head/UPDATING head/sys/amd64/amd64/machdep.c head/sys/amd64/amd64/mp_machdep.c head/sys/amd64/include/smp.h head/sys/i386/i386/machdep.c head/sys/i386/i386/mp_machdep.c head/sys/i386/include/smp.h head/sys/pc98/pc98/machdep.c Modified: head/UPDATING ============================================================================== --- head/UPDATING Wed Jun 8 08:08:42 2011 (r222852) +++ head/UPDATING Wed Jun 8 08:12:15 2011 (r222853) @@ -22,6 +22,23 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 9. machines to maximize performance. (To disable malloc debugging, run ln -s aj /etc/malloc.conf.) +20110608: + The following sysctls and tunables are retired on x86 platforms: + machdep.hlt_cpus + machdep.hlt_logical_cpus + The following sysctl is retired: + machdep.hyperthreading_allowed + The sysctls were supposed to provide a way to dynamically offline and + online selected CPUs on x86 platforms, but the implementation has not + been reliable especially with SCHED_ULE scheduler. + machdep.hyperthreading_allowed tunable is still available to ignore + hyperthreading CPUs at OS level. + Individual CPUs can be disabled using hint.lapic.X.disabled tunable, + where X is an APIC ID of a CPU. Be advised, though, that disabling + CPUs in non-uniform fashion will result in non-uniform topology and + may lead to sub-optimal system performance with SCHED_ULE, which is + a default scheduler. + 20110607: cpumask_t type is retired and cpuset_t is used in order to describe a mask of CPUs. Modified: head/sys/amd64/amd64/machdep.c ============================================================================== --- head/sys/amd64/amd64/machdep.c Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/amd64/amd64/machdep.c Wed Jun 8 08:12:15 2011 (r222853) @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); #include "opt_isa.h" #include "opt_kstack_pages.h" #include "opt_maxmem.h" +#include "opt_mp_watchdog.h" #include "opt_perfmon.h" #include "opt_sched.h" #include "opt_kdtrace.h" @@ -116,6 +117,7 @@ __FBSDID("$FreeBSD$"); #include <x86/mca.h> #include <machine/md_var.h> #include <machine/metadata.h> +#include <machine/mp_watchdog.h> #include <machine/pc/bios.h> #include <machine/pcb.h> #include <machine/proc.h> @@ -734,9 +736,8 @@ cpu_idle(int busy) CTR2(KTR_SPARE2, "cpu_idle(%d) at %d", busy, curcpu); -#ifdef SMP - if (mp_grab_cpu_hlt()) - return; +#ifdef MP_WATCHDOG + ap_watchdog(PCPU_GET(cpuid)); #endif /* If we are busy - try to use fast methods. */ if (busy) { Modified: head/sys/amd64/amd64/mp_machdep.c ============================================================================== --- head/sys/amd64/amd64/mp_machdep.c Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/amd64/amd64/mp_machdep.c Wed Jun 8 08:12:15 2011 (r222853) @@ -29,7 +29,6 @@ __FBSDID("$FreeBSD$"); #include "opt_cpu.h" #include "opt_kstack_pages.h" -#include "opt_mp_watchdog.h" #include "opt_sched.h" #include "opt_smp.h" @@ -64,7 +63,6 @@ __FBSDID("$FreeBSD$"); #include <machine/cpufunc.h> #include <x86/mca.h> #include <machine/md_var.h> -#include <machine/mp_watchdog.h> #include <machine/pcb.h> #include <machine/psl.h> #include <machine/smp.h> @@ -160,11 +158,8 @@ static int start_all_aps(void); static int start_ap(int apic_id); static void release_aps(void *dummy); -static int hlt_logical_cpus; static u_int hyperthreading_cpus; /* logical cpus sharing L1 cache */ -static cpuset_t hyperthreading_cpus_mask; static int hyperthreading_allowed = 1; -static struct sysctl_ctx_list logical_cpu_clist; static u_int bootMP_size; static void @@ -748,11 +743,6 @@ init_secondary(void) if (cpu_logical > 1 && PCPU_GET(apic_id) % cpu_logical != 0) CPU_OR(&logical_cpus_mask, &tcpuset); - /* Determine if we are a hyperthread. */ - if (hyperthreading_cpus > 1 && - PCPU_GET(apic_id) % hyperthreading_cpus != 0) - CPU_OR(&hyperthreading_cpus_mask, &tcpuset); - /* Build our map of 'other' CPUs. */ tallcpus = all_cpus; CPU_NAND(&tallcpus, &tcpuset); @@ -843,7 +833,7 @@ assign_cpu_ids(void) if (hyperthreading_cpus > 1 && i % hyperthreading_cpus != 0) { cpu_info[i].cpu_hyperthread = 1; -#if defined(SCHED_ULE) + /* * Don't use HT CPU if it has been disabled by a * tunable. @@ -852,7 +842,6 @@ assign_cpu_ids(void) cpu_info[i].cpu_disabled = 1; continue; } -#endif } /* Don't use this CPU if it has been disabled by a tunable. */ @@ -862,6 +851,11 @@ assign_cpu_ids(void) } } + if (hyperthreading_allowed == 0 && hyperthreading_cpus > 1) { + hyperthreading_cpus = 0; + cpu_logical = 1; + } + /* * Assign CPU IDs to local APIC IDs and disable any CPUs * beyond MAXCPU. CPU 0 is always assigned to the BSP. @@ -1487,159 +1481,6 @@ release_aps(void *dummy __unused) } SYSINIT(start_aps, SI_SUB_SMP, SI_ORDER_FIRST, release_aps, NULL); -static int -sysctl_hlt_cpus(SYSCTL_HANDLER_ARGS) -{ - cpuset_t mask; - int error; - - mask = hlt_cpus_mask; - error = sysctl_handle_opaque(oidp, &mask, sizeof(mask), req); - if (error || !req->newptr) - return (error); - - if (!CPU_EMPTY(&logical_cpus_mask) && - CPU_SUBSET(&mask, &logical_cpus_mask)) - hlt_logical_cpus = 1; - else - hlt_logical_cpus = 0; - - if (! hyperthreading_allowed) - CPU_OR(&mask, &hyperthreading_cpus_mask); - - if (CPU_SUBSET(&mask, &all_cpus)) - CPU_CLR(0, &mask); - hlt_cpus_mask = mask; - return (error); -} -SYSCTL_PROC(_machdep, OID_AUTO, hlt_cpus, - CTLTYPE_STRUCT | CTLFLAG_RW | CTLFLAG_MPSAFE, 0, 0, sysctl_hlt_cpus, "S", - "Bitmap of CPUs to halt. 101 (binary) will halt CPUs 0 and 2."); - -static int -sysctl_hlt_logical_cpus(SYSCTL_HANDLER_ARGS) -{ - int disable, error; - - disable = hlt_logical_cpus; - error = sysctl_handle_int(oidp, &disable, 0, req); - if (error || !req->newptr) - return (error); - - if (disable) - CPU_OR(&hlt_cpus_mask, &logical_cpus_mask); - else - CPU_NAND(&hlt_cpus_mask, &logical_cpus_mask); - - if (! hyperthreading_allowed) - CPU_OR(&hlt_cpus_mask, &hyperthreading_cpus_mask); - - if (CPU_SUBSET(&hlt_cpus_mask, &all_cpus)) - CPU_CLR(0, &hlt_cpus_mask); - - hlt_logical_cpus = disable; - return (error); -} - -static int -sysctl_hyperthreading_allowed(SYSCTL_HANDLER_ARGS) -{ - int allowed, error; - - allowed = hyperthreading_allowed; - error = sysctl_handle_int(oidp, &allowed, 0, req); - if (error || !req->newptr) - return (error); - -#ifdef SCHED_ULE - /* - * SCHED_ULE doesn't allow enabling/disabling HT cores at - * run-time. - */ - if (allowed != hyperthreading_allowed) - return (ENOTSUP); - return (error); -#endif - - if (allowed) - CPU_NAND(&hlt_cpus_mask, &hyperthreading_cpus_mask); - else - CPU_OR(&hlt_cpus_mask, &hyperthreading_cpus_mask); - - if (!CPU_EMPTY(&logical_cpus_mask) && - CPU_SUBSET(&hlt_cpus_mask, &logical_cpus_mask)) - hlt_logical_cpus = 1; - else - hlt_logical_cpus = 0; - - if (CPU_SUBSET(&hlt_cpus_mask, &all_cpus)) - CPU_CLR(0, &hlt_cpus_mask); - - hyperthreading_allowed = allowed; - return (error); -} - -static void -cpu_hlt_setup(void *dummy __unused) -{ - - if (!CPU_EMPTY(&logical_cpus_mask)) { - TUNABLE_INT_FETCH("machdep.hlt_logical_cpus", - &hlt_logical_cpus); - sysctl_ctx_init(&logical_cpu_clist); - SYSCTL_ADD_PROC(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "hlt_logical_cpus", CTLTYPE_INT|CTLFLAG_RW, 0, 0, - sysctl_hlt_logical_cpus, "IU", ""); - SYSCTL_ADD_UINT(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "logical_cpus_mask", CTLTYPE_INT|CTLFLAG_RD, - &logical_cpus_mask, 0, ""); - - if (hlt_logical_cpus) - CPU_OR(&hlt_cpus_mask, &logical_cpus_mask); - - /* - * If necessary for security purposes, force - * hyperthreading off, regardless of the value - * of hlt_logical_cpus. - */ - if (!CPU_EMPTY(&hyperthreading_cpus_mask)) { - SYSCTL_ADD_PROC(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "hyperthreading_allowed", CTLTYPE_INT|CTLFLAG_RW, - 0, 0, sysctl_hyperthreading_allowed, "IU", ""); - if (! hyperthreading_allowed) - CPU_OR(&hlt_cpus_mask, - &hyperthreading_cpus_mask); - } - } -} -SYSINIT(cpu_hlt, SI_SUB_SMP, SI_ORDER_ANY, cpu_hlt_setup, NULL); - -int -mp_grab_cpu_hlt(void) -{ - cpuset_t mask; -#ifdef MP_WATCHDOG - u_int cpuid; -#endif - int retval; - - mask = PCPU_GET(cpumask); -#ifdef MP_WATCHDOG - cpuid = PCPU_GET(cpuid); - ap_watchdog(cpuid); -#endif - - retval = 0; - while (CPU_OVERLAP(&mask, &hlt_cpus_mask)) { - retval = 1; - __asm __volatile("sti; hlt" : : : "memory"); - } - return (retval); -} - #ifdef COUNT_IPIS /* * Setup interrupt counters for IPI handlers. Modified: head/sys/amd64/include/smp.h ============================================================================== --- head/sys/amd64/include/smp.h Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/amd64/include/smp.h Wed Jun 8 08:12:15 2011 (r222853) @@ -65,7 +65,6 @@ void ipi_cpu(int cpu, u_int ipi); int ipi_nmi_handler(void); void ipi_selected(cpuset_t cpus, u_int ipi); u_int mp_bootaddress(u_int); -int mp_grab_cpu_hlt(void); void smp_cache_flush(void); void smp_invlpg(vm_offset_t addr); void smp_masked_invlpg(cpuset_t mask, vm_offset_t addr); Modified: head/sys/i386/i386/machdep.c ============================================================================== --- head/sys/i386/i386/machdep.c Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/i386/i386/machdep.c Wed Jun 8 08:12:15 2011 (r222853) @@ -49,6 +49,7 @@ __FBSDID("$FreeBSD$"); #include "opt_isa.h" #include "opt_kstack_pages.h" #include "opt_maxmem.h" +#include "opt_mp_watchdog.h" #include "opt_npx.h" #include "opt_perfmon.h" #include "opt_xbox.h" @@ -118,6 +119,7 @@ __FBSDID("$FreeBSD$"); #include <x86/mca.h> #include <machine/md_var.h> #include <machine/metadata.h> +#include <machine/mp_watchdog.h> #include <machine/pc/bios.h> #include <machine/pcb.h> #include <machine/pcb_ext.h> @@ -1357,9 +1359,8 @@ cpu_idle(int busy) CTR2(KTR_SPARE2, "cpu_idle(%d) at %d", busy, curcpu); -#if defined(SMP) && !defined(XEN) - if (mp_grab_cpu_hlt()) - return; +#if defined(MP_WATCHDOG) && !defined(XEN) + ap_watchdog(PCPU_GET(cpuid)); #endif #ifndef XEN /* If we are busy - try to use fast methods. */ Modified: head/sys/i386/i386/mp_machdep.c ============================================================================== --- head/sys/i386/i386/mp_machdep.c Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/i386/i386/mp_machdep.c Wed Jun 8 08:12:15 2011 (r222853) @@ -29,7 +29,6 @@ __FBSDID("$FreeBSD$"); #include "opt_apic.h" #include "opt_cpu.h" #include "opt_kstack_pages.h" -#include "opt_mp_watchdog.h" #include "opt_pmap.h" #include "opt_sched.h" #include "opt_smp.h" @@ -78,7 +77,6 @@ __FBSDID("$FreeBSD$"); #include <machine/cputypes.h> #include <x86/mca.h> #include <machine/md_var.h> -#include <machine/mp_watchdog.h> #include <machine/pcb.h> #include <machine/psl.h> #include <machine/smp.h> @@ -209,11 +207,8 @@ static int start_all_aps(void); static int start_ap(int apic_id); static void release_aps(void *dummy); -static int hlt_logical_cpus; static u_int hyperthreading_cpus; /* logical cpus sharing L1 cache */ -static cpuset_t hyperthreading_cpus_mask; static int hyperthreading_allowed = 1; -static struct sysctl_ctx_list logical_cpu_clist; static void mem_range_AP_init(void) @@ -794,11 +789,6 @@ init_secondary(void) /* XXX Calculation depends on cpu_logical being a power of 2, e.g. 2 */ if (cpu_logical > 1 && PCPU_GET(apic_id) % cpu_logical != 0) CPU_OR(&logical_cpus_mask, &tcpuset); - - /* Determine if we are a hyperthread. */ - if (hyperthreading_cpus > 1 && - PCPU_GET(apic_id) % hyperthreading_cpus != 0) - CPU_OR(&hyperthreading_cpus_mask, &tcpuset); /* Build our map of 'other' CPUs. */ tallcpus = all_cpus; @@ -882,7 +872,7 @@ assign_cpu_ids(void) if (hyperthreading_cpus > 1 && i % hyperthreading_cpus != 0) { cpu_info[i].cpu_hyperthread = 1; -#if defined(SCHED_ULE) + /* * Don't use HT CPU if it has been disabled by a * tunable. @@ -891,7 +881,6 @@ assign_cpu_ids(void) cpu_info[i].cpu_disabled = 1; continue; } -#endif } /* Don't use this CPU if it has been disabled by a tunable. */ @@ -901,6 +890,11 @@ assign_cpu_ids(void) } } + if (hyperthreading_allowed == 0 && hyperthreading_cpus > 1) { + hyperthreading_cpus = 0; + cpu_logical = 1; + } + /* * Assign CPU IDs to local APIC IDs and disable any CPUs * beyond MAXCPU. CPU 0 is always assigned to the BSP. @@ -1550,159 +1544,6 @@ release_aps(void *dummy __unused) } SYSINIT(start_aps, SI_SUB_SMP, SI_ORDER_FIRST, release_aps, NULL); -static int -sysctl_hlt_cpus(SYSCTL_HANDLER_ARGS) -{ - cpuset_t mask; - int error; - - mask = hlt_cpus_mask; - error = sysctl_handle_opaque(oidp, &mask, sizeof(mask), req); - if (error || !req->newptr) - return (error); - - if (!CPU_EMPTY(&logical_cpus_mask) && - CPU_SUBSET(&mask, &logical_cpus_mask)) - hlt_logical_cpus = 1; - else - hlt_logical_cpus = 0; - - if (! hyperthreading_allowed) - CPU_OR(&mask, &hyperthreading_cpus_mask); - - if (CPU_SUBSET(&mask, &all_cpus)) - CPU_CLR(0, &mask); - hlt_cpus_mask = mask; - return (error); -} -SYSCTL_PROC(_machdep, OID_AUTO, hlt_cpus, - CTLTYPE_STRUCT | CTLFLAG_RW | CTLFLAG_MPSAFE, 0, 0, sysctl_hlt_cpus, "S", - "Bitmap of CPUs to halt. 101 (binary) will halt CPUs 0 and 2."); - -static int -sysctl_hlt_logical_cpus(SYSCTL_HANDLER_ARGS) -{ - int disable, error; - - disable = hlt_logical_cpus; - error = sysctl_handle_int(oidp, &disable, 0, req); - if (error || !req->newptr) - return (error); - - if (disable) - CPU_OR(&hlt_cpus_mask, &logical_cpus_mask); - else - CPU_NAND(&hlt_cpus_mask, &logical_cpus_mask); - - if (! hyperthreading_allowed) - CPU_OR(&hlt_cpus_mask, &hyperthreading_cpus_mask); - - if (CPU_SUBSET(&hlt_cpus_mask, &all_cpus)) - CPU_CLR(0, &hlt_cpus_mask); - - hlt_logical_cpus = disable; - return (error); -} - -static int -sysctl_hyperthreading_allowed(SYSCTL_HANDLER_ARGS) -{ - int allowed, error; - - allowed = hyperthreading_allowed; - error = sysctl_handle_int(oidp, &allowed, 0, req); - if (error || !req->newptr) - return (error); - -#ifdef SCHED_ULE - /* - * SCHED_ULE doesn't allow enabling/disabling HT cores at - * run-time. - */ - if (allowed != hyperthreading_allowed) - return (ENOTSUP); - return (error); -#endif - - if (allowed) - CPU_NAND(&hlt_cpus_mask, &hyperthreading_cpus_mask); - else - CPU_OR(&hlt_cpus_mask, &hyperthreading_cpus_mask); - - if (!CPU_EMPTY(&logical_cpus_mask) && - CPU_SUBSET(&hlt_cpus_mask, &logical_cpus_mask)) - hlt_logical_cpus = 1; - else - hlt_logical_cpus = 0; - - if (CPU_SUBSET(&hlt_cpus_mask, &all_cpus)) - CPU_CLR(0, &hlt_cpus_mask); - - hyperthreading_allowed = allowed; - return (error); -} - -static void -cpu_hlt_setup(void *dummy __unused) -{ - - if (!CPU_EMPTY(&logical_cpus_mask)) { - TUNABLE_INT_FETCH("machdep.hlt_logical_cpus", - &hlt_logical_cpus); - sysctl_ctx_init(&logical_cpu_clist); - SYSCTL_ADD_PROC(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "hlt_logical_cpus", CTLTYPE_INT|CTLFLAG_RW, 0, 0, - sysctl_hlt_logical_cpus, "IU", ""); - SYSCTL_ADD_UINT(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "logical_cpus_mask", CTLTYPE_INT|CTLFLAG_RD, - &logical_cpus_mask, 0, ""); - - if (hlt_logical_cpus) - CPU_OR(&hlt_cpus_mask, &logical_cpus_mask); - - /* - * If necessary for security purposes, force - * hyperthreading off, regardless of the value - * of hlt_logical_cpus. - */ - if (!CPU_EMPTY(&hyperthreading_cpus_mask)) { - SYSCTL_ADD_PROC(&logical_cpu_clist, - SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO, - "hyperthreading_allowed", CTLTYPE_INT|CTLFLAG_RW, - 0, 0, sysctl_hyperthreading_allowed, "IU", ""); - if (! hyperthreading_allowed) - CPU_OR(&hlt_cpus_mask, - &hyperthreading_cpus_mask); - } - } -} -SYSINIT(cpu_hlt, SI_SUB_SMP, SI_ORDER_ANY, cpu_hlt_setup, NULL); - -int -mp_grab_cpu_hlt(void) -{ - cpuset_t mask; -#ifdef MP_WATCHDOG - u_int cpuid; -#endif - int retval; - - mask = PCPU_GET(cpumask); -#ifdef MP_WATCHDOG - cpuid = PCPU_GET(cpuid); - ap_watchdog(cpuid); -#endif - - retval = 0; - while (CPU_OVERLAP(&mask, &hlt_cpus_mask)) { - retval = 1; - __asm __volatile("sti; hlt" : : : "memory"); - } - return (retval); -} - #ifdef COUNT_IPIS /* * Setup interrupt counters for IPI handlers. Modified: head/sys/i386/include/smp.h ============================================================================== --- head/sys/i386/include/smp.h Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/i386/include/smp.h Wed Jun 8 08:12:15 2011 (r222853) @@ -68,7 +68,6 @@ void ipi_cpu(int cpu, u_int ipi); int ipi_nmi_handler(void); void ipi_selected(cpuset_t cpus, u_int ipi); u_int mp_bootaddress(u_int); -int mp_grab_cpu_hlt(void); void smp_cache_flush(void); void smp_invlpg(vm_offset_t addr); void smp_masked_invlpg(cpuset_t mask, vm_offset_t addr); Modified: head/sys/pc98/pc98/machdep.c ============================================================================== --- head/sys/pc98/pc98/machdep.c Wed Jun 8 08:08:42 2011 (r222852) +++ head/sys/pc98/pc98/machdep.c Wed Jun 8 08:12:15 2011 (r222853) @@ -49,6 +49,7 @@ __FBSDID("$FreeBSD$"); #include "opt_isa.h" #include "opt_kstack_pages.h" #include "opt_maxmem.h" +#include "opt_mp_watchdog.h" #include "opt_npx.h" #include "opt_perfmon.h" @@ -115,6 +116,7 @@ __FBSDID("$FreeBSD$"); #include <machine/intr_machdep.h> #include <x86/mca.h> #include <machine/md_var.h> +#include <machine/mp_watchdog.h> #include <machine/pc/bios.h> #include <machine/pcb.h> #include <machine/pcb_ext.h> @@ -1193,9 +1195,8 @@ cpu_idle(int busy) CTR2(KTR_SPARE2, "cpu_idle(%d) at %d", busy, curcpu); -#ifdef SMP - if (mp_grab_cpu_hlt()) - return; +#ifdef MP_WATCHDOG + ap_watchdog(PCPU_GET(cpuid)); #endif /* If we are busy - try to use fast methods. */ if (busy) { _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State Changed From-To: open->closed The (broken) functionality referenced in this PR has been removed until it is re-implemented properly. Responsible Changed From-To: freebsd-bugs->avg I am the one who touched the code. |