Date: Thu, 26 May 2011 08:46:58 -0700 From: mdf@FreeBSD.org To: Andriy Gapon <avg@freebsd.org> Cc: Attilio Rao <attilio@freebsd.org>, src-committers@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>, svn-src-user@freebsd.org Subject: Re: svn commit: r222060 - in user/avg/xcpu/sys: kern sys Message-ID: <BANLkTim_zEDPANqZTpyYdOKqDaPEc8EhVg@mail.gmail.com> In-Reply-To: <4DDE7555.7090500@FreeBSD.org> References: <201105181508.p4IF8UoS096841@svn.freebsd.org> <20110518182441.GB2273@garage.freebsd.pl> <4DD4243C.4070301@FreeBSD.org> <BANLkTikAnB-3XbvDwGHgyqyJquH9BhqzOQ@mail.gmail.com> <4DDD13F9.5040800@FreeBSD.org> <4DDE7555.7090500@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 26, 2011 at 8:44 AM, Andriy Gapon <avg@freebsd.org> wrote: > on 25/05/2011 17:36 Andriy Gapon said the following: >> on 18/05/2011 23:06 Attilio Rao said the following: >>> However I think that TDF_INPANIC handling is not optimal. >>> You should really acquire thread_lock otherwise you are going to break >>> choosethread() concurrency. >>> >>> I would prefer to make TDF_INPANIC a private flag and just use it with >>> curthread, if possible, but I still don't have a good way to resolve >>> choosethread() (I would dig the runqueue adding path and resolve the >>> problem later in the codeflow, I think). >> >> I've been thinking about this. >> I think that in the new world where only one thread runs after panic we = could just >> reduce TD_IS_INPANIC to panicstr !=3D NULL, TDF_INPANIC could be removed= altogether >> along with the check in =A0choosethread(). =A0But for some initial perio= d I would like >> to have an option to disable CPU stopping (to protect from possible bugs= , >> regressions, etc) and for that I would like to keep TDF_INPANIC. =A0The = flag could >> be set without thread_lock() because we still allow only one thread to b= e in/after >> panic. =A0But I completely agree with you that it is cleaner to move TDF= _INPANIC to >> private flags. >> >> So the first step: >> TDF_INPANIC =3D> to private flags >> >> Some time in the future: >> TDF_INPANIC =3D> removed >> TD_IS_INPANIC =3D> panicstr !=3D NULL >> > > Ehm... =A0After discussing this issue with you on IRC I realized absurdit= y of my > interim suggestion. > > New proposal: > #define TD_IS_INPANIC() \ > =A0 =A0 =A0 =A0(panicstr !=3D NULL && stop_cpus_on_panic) > > When/if stop_cpus_on_panic knob is removed, then TD_IS_INPANIC will natur= ally be > reduced to (panicstr !=3D NULL) and TDF_INPANIC flag will also go as we w= ill be > guaranteed that the scheduler will not be running. > > Given the above, maybe TD_IS_INPANIC should change name again as it doesn= 't check > properties of a particular thread, but rather the whole system state? =A0= Also, > sys/proc.h doesn't seem like the best location for it anymore. A per-thread flag is needed as long as other CPUs can be running or even just the scheduler on the remaining CPU. So I would thing that flag needs to be checked until the system has been massaged to the state you describe above. Cheers, matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTim_zEDPANqZTpyYdOKqDaPEc8EhVg>