Date: Wed, 29 Nov 2017 19:42:10 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Justin Hibbits <jrh29@alumni.cwru.edu>, Konstantin Belousov <kostikbel@gmail.com> Cc: John Baldwin <jhb@freebsd.org>, Nathan Whitehorn <nwhitehorn@freebsd.org>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, "O. Hartmann" <ohartmann@walstatt.org>, KrishnamRaju ErapaRaju <Krishna2@chelsio.com> Subject: Re: svn commit: r326218 - head/sys/kern Message-ID: <0baa25c3-f235-78c5-5c72-9eab5dab89c6@selasky.org> In-Reply-To: <CAHSQbTAfQcudcVRUGR=wwm9hvbgvhaFuTVjA8D15C%2B8fRfN=hw@mail.gmail.com> References: <201711252341.vAPNf5Qx001464@repo.freebsd.org> <14322447.103fKFTi3y@ralph.baldwin.cx> <3fc45d5f-22b9-0562-278b-c515e36f48e7@freebsd.org> <14058479.lc6xlYgyBM@ralph.baldwin.cx> <2e752d6a-d34a-dde0-acea-fdbf8bebea51@selasky.org> <20171129161356.GA2272@kib.kiev.ua> <CAHSQbTAfQcudcVRUGR=wwm9hvbgvhaFuTVjA8D15C%2B8fRfN=hw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/29/17 17:59, Justin Hibbits wrote:
> On Wed, Nov 29, 2017 at 10:13 AM, Konstantin Belousov
> <kostikbel@gmail.com> wrote:
>> On Wed, Nov 29, 2017 at 04:33:50PM +0100, Hans Petter Selasky wrote:
>>> Hi Nathan,
>>>
>>> The chunk below causes sched_pin() to stop working and should be removed
>>> from your commit ??!!
>>>
>>> It probably explains the hangs seen recently reported by various brave
>>> people running 12-current :-)
>>>
>>> Specifically I see threads migrating between CPUs when td->td_pinned > 0
>>> using the LinuxKPI RCU API, which in turn leads to a hang when trying to
>>> synchronize RCU.
>>>
>>> --HPS
>>>
>>> diff --git a/sys/kern/sched_ule.c b/sys/kern/sched_ule.c
>>> index 5c8bae5afa1..bd4b505f6c3 100644
>>> --- a/sys/kern/sched_ule.c
>>> +++ b/sys/kern/sched_ule.c
>>> @@ -2453,7 +2453,7 @@ sched_add(struct thread *td, int flags)
>>> * Pick the destination cpu and if it isn't ours transfer to the
>>> * target cpu.
>>> */
>>> - td_get_sched(td)->ts_cpu = curcpu; /* Pick something valid to
>>> start */
>>> +// td_get_sched(td)->ts_cpu = curcpu; /* Pick something valid to
>>> start */
>>> cpu = sched_pickcpu(td, flags);
>>> tdq = sched_setcpu(td, cpu, flags);
>>> tdq_add(tdq, td, flags);
>>
>> To clarify. It seems that this change breaks sched_bind().
>>
>> It might be that LinuxKPI does use sched_pin() in somewhat questionable
>> way. Namely, the code puts the thread off the CPU (e.g. by taking a
>> lock). Then, is it guaranteed that the pinned thread returns to the same
>> cpu after sched_add() ?
>>
>> I think that the second behaviour is not guaranteed, but it might
>> happens by the way the things are arranged. If guaranteed, then the
>> sched_pin() breakage is same as for sched_bind().
>>
>
> I see the same breakage on PowerPC with the dtsec(4) driver, which
> pins interrupts to CPUs, now causing interrupts to migrate to cores
> without the matching portal mapping.
>
> - Justin
Hi Nathan,
Looking at your patch I think what you wanted to do is something like
shown below. I'm not sure if sched_setup() is run early enough to catch
all created threads in the registered eventhandler hook. I'll leave the
final solution and analysis up to you.
--HPS
> diff --git a/sys/kern/sched_ule.c b/sys/kern/sched_ule.c
> index 5c8bae5afa1..42ccf58ea0c 100644
> --- a/sys/kern/sched_ule.c
> +++ b/sys/kern/sched_ule.c
> @@ -1387,6 +1387,13 @@ sched_setup_smp(void)
> }
> #endif
>
> +static void
> +sched_thread_ctor(void *arg __unused, struct thread *td)
> +{
> + /* Pick a valid initial CPU number */
> + td_get_sched(td)->ts_cpu = curcpu;
> +}
> +
> /*
> * Setup the thread queues and initialize the topology based on MD
> * information.
> @@ -1396,6 +1403,9 @@ sched_setup(void *dummy)
> {
> struct tdq *tdq;
>
> + (void) EVENTHANDLER_REGISTER(thread_ctor,
> + sched_thread_ctor, NULL, EVENTHANDLER_PRI_ANY);
> +
> tdq = TDQ_SELF();
> #ifdef SMP
> sched_setup_smp();
> @@ -2453,7 +2463,6 @@ sched_add(struct thread *td, int flags)
> * Pick the destination cpu and if it isn't ours transfer to the
> * target cpu.
> */
> - td_get_sched(td)->ts_cpu = curcpu; /* Pick something valid to start */
> cpu = sched_pickcpu(td, flags);
> tdq = sched_setcpu(td, cpu, flags);
> tdq_add(tdq, td, flags);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0baa25c3-f235-78c5-5c72-9eab5dab89c6>