From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56342) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1coyiJ-0007KW-Kl for qemu-devel@nongnu.org; Fri, 17 Mar 2017 16:43:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1coyiF-0005pR-MT for qemu-devel@nongnu.org; Fri, 17 Mar 2017 16:43:07 -0400 Received: from mail-wm0-x230.google.com ([2a00:1450:400c:c09::230]:36813) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1coyiF-0005p2-Cv for qemu-devel@nongnu.org; Fri, 17 Mar 2017 16:43:03 -0400 Received: by mail-wm0-x230.google.com with SMTP id n11so24211903wma.1 for ; Fri, 17 Mar 2017 13:43:02 -0700 (PDT) References: <20170224112109.3147-1-alex.bennee@linaro.org> <20170224112109.3147-12-alex.bennee@linaro.org> <87y3wr8zs2.fsf@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Fri, 17 Mar 2017 20:43:22 +0000 Message-ID: <87var7ocr9.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PULL 11/24] tcg: enable thread-per-vCPU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: peter.maydell@linaro.org, Peter Crosthwaite , qemu-devel@nongnu.org, Paolo Bonzini , KONRAD Frederic , Richard Henderson Laurent Vivier writes: > Le 27/02/2017 à 15:38, Alex Bennée a écrit : >> >> Laurent Vivier writes: >> >>> Le 24/02/2017 à 12:20, Alex Bennée a écrit : >>>> There are a couple of changes that occur at the same time here: >>>> >>>> - introduce a single vCPU qemu_tcg_cpu_thread_fn >>>> >>>> One of these is spawned per vCPU with its own Thread and Condition >>>> variables. qemu_tcg_rr_cpu_thread_fn is the new name for the old >>>> single threaded function. >>>> >>>> - the TLS current_cpu variable is now live for the lifetime of MTTCG >>>> vCPU threads. This is for future work where async jobs need to know >>>> the vCPU context they are operating in. >>>> >>>> The user to switch on multi-thread behaviour and spawn a thread >>>> per-vCPU. For a simple test kvm-unit-test like: >>>> >>>> ./arm/run ./arm/locking-test.flat -smp 4 -accel tcg,thread=multi >>>> >>>> Will now use 4 vCPU threads and have an expected FAIL (instead of the >>>> unexpected PASS) as the default mode of the test has no protection when >>>> incrementing a shared variable. >>>> >>>> We enable the parallel_cpus flag to ensure we generate correct barrier >>>> and atomic code if supported by the front and backends. This doesn't >>>> automatically enable MTTCG until default_mttcg_enabled() is updated to >>>> check the configuration is supported. >>> >>> This commit breaks linux-user mode: >>> >>> debian-8 with qemu-ppc on x86_64 with ltp-full-20170116 >>> >>> cd /opt/ltp >>> ./runltp -p -l "qemu-$(date +%FT%T).log" -f /opt/ltp/runtest/syscalls -s >>> setgroups03 >>> >>> setgroups03 1 TPASS : setgroups(65537) fails, Size is > >>> sysconf(_SC_NGROUPS_MAX), errno=22 >>> qemu-ppc: /home/laurent/Projects/qemu/include/qemu/rcu.h:89: >>> rcu_read_unlock: Assertion `p_rcu_reader->depth != 0' failed. >>> qemu-ppc: /home/laurent/Projects/qemu/include/qemu/rcu.h:89: >>> rcu_read_unlock: Assertion `p_rcu_reader->depth != 0' failed. >>> qemu-ppc: /home/laurent/Projects/qemu/include/qemu/rcu.h:89: >>> rcu_read_unlock: Assertion `p_rcu_reader->depth != 0' failed. >>> ... >> >> Interesting. I can only think the current_cpu change has broken it >> because most of the changes in this commit affect softmmu targets only >> (linux-user has its own run loop). >> >> Thanks for the report - I'll look into it. > > After: > > 95b0eca Merge remote-tracking branch > 'remotes/stsquad/tags/pull-mttcg-fixups-090317-1' into staging > > [Tested with my HEAD on: > b1616fe Merge remote-tracking branch > 'remotes/famz/tags/docker-pull-request' into staging] > > I have now: > > <<>> > tag=setgroups03 stime=1489413401 > cmdline="setgroups03" > contacts="" > analysis=exit > <<>> > ** > ERROR:/home/laurent/Projects/qemu/cpu-exec.c:656:cpu_exec: assertion > failed: (cpu == current_cpu) > ** OK we now understand what's happening: - setgroups calls __nptl_setxid_error, triggers abort() - this sends sig_num 6, then 11 - host_signal_handler tries to handle 11 - -> handle_cpu_signal Pre: tcg: enable thread-per-vCPU caused this problem: - current_cpu was reset to NULL on the way out of the loop - therefore handle_cpu_signal went boom because cpu = current_cpu; cc = CPU_GET_CLASS(cpu); Post: tcg: enable thread-per-vCPU caused this problem: - current_cpu is now live outside cpu_exec_loop - this is mainly so async_work functions can assert (cpu == current_cpu) - hence handle_cpu_signal gets further and calls cpu_loop_exit(cpu); - hilarity ensues as we siglongjmp into a stale context Obviously we shouldn't try to siglongjmp. But we also shouldn't rely on current_cpu as a proxy to crash early when outside of the loop. There is a slight wrinkle that we also have funny handling of segs during translation if a guest jumps to code in an as-yet un-mapped region of memory. There is currently cpu->running which is set/cleared by cpu_exec_start/end. Although if we crash between cpu_exec_start and sigsetjmp the same sort of brokenness might happen. Anyway understood now. If anyone has any suggestions for neater stuff over the weekend please shout, otherwise I'll probably just hack handle_cpu_signal to do: cpu = current_cpu; if (!cpu->running) { /* we weren't running or translating JIT code when the signal came */ return 1; } -- Alex Bennée