From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752306AbcA0BPF (ORCPT ); Tue, 26 Jan 2016 20:15:05 -0500 Received: from mail-pa0-f46.google.com ([209.85.220.46]:33330 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751494AbcA0BO7 (ORCPT ); Tue, 26 Jan 2016 20:14:59 -0500 Subject: Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection To: Ricky Liang References: <1449641971-20827-1-git-send-email-smuckle@linaro.org> <1449641971-20827-4-git-send-email-smuckle@linaro.org> Cc: Peter Zijlstra , Ingo Molnar , open list , linux-pm@vger.kernel.org, Vincent Guittot , Morten Rasmussen , Dietmar Eggemann , Juri Lelli , Patrick Bellasi , Michael Turquette From: Steve Muckle Message-ID: <56A81A10.4020802@linaro.org> Date: Tue, 26 Jan 2016 17:14:56 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ricky, On 01/25/2016 04:06 AM, Ricky Liang wrote: >> + do { >> + set_current_state(TASK_INTERRUPTIBLE); >> + new_request = gd->requested_freq; >> + if (new_request == last_request) { >> + schedule(); > > Should we check kthread_should_stop() after > set_current_state(TASK_INTERRUPTIBLE), probably right before > schedule()? Something like: > > set_current_state(TASK_INTERRUPTIBLE); > new_request = gd->requested_freq; > if (new_request == last_request) { > if (kthread_should_stop()) > break; > schedule(); > } else { > ... > } > > On the previous version of the scheduler-driver cpu frequency > selection I had the following: > > <3>[ 1920.233598] INFO: task autotest:32443 blocked for more than 120 seconds. > <3>[ 1920.233625] Not tainted 3.18.0-09696-g4312b25 #1 > <3>[ 1920.233641] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > disables this message. > <6>[ 1920.233659] autotest D ffffffc0002057a0 0 32443 > 32403 0x00400000 > <0>[ 1920.233693] Call trace: > <4>[ 1920.233724] [] __switch_to+0x80/0x8c > <4>[ 1920.233748] [] __schedule+0x550/0x7d8 > <4>[ 1920.233769] [] schedule+0x78/0x84 > <4>[ 1920.233786] [] schedule_timeout+0x40/0x2ac > <4>[ 1920.233804] [] wait_for_common+0x154/0x18c > <4>[ 1920.233820] [] wait_for_completion+0x24/0x34 > <4>[ 1920.233840] [] kthread_stop+0x130/0x22c > <4>[ 1920.233859] [] cpufreq_sched_setup+0x21c/0x308 > <4>[ 1920.233881] [] __cpufreq_governor+0x114/0x1c8 > <4>[ 1920.233901] [] cpufreq_set_policy+0x120/0x1b8 > <4>[ 1920.233920] [] store_scaling_governor+0x8c/0xd4 > <4>[ 1920.233937] [] store+0x98/0xd0 > <4>[ 1920.233958] [] sysfs_kf_write+0x54/0x64 > <4>[ 1920.233977] [] kernfs_fop_write+0x108/0x150 > <4>[ 1920.233999] [] vfs_write+0xc4/0x1a0 > <4>[ 1920.234018] [] SyS_write+0x60/0xb4 > <4>[ 1920.234031] INFO: lockdep is turned off. > <6>[ 1920.234043] task PC stack pid father > <6>[ 1920.234161] autotest D ffffffc0002057a0 0 32443 > 32403 0x00400000 > <0>[ 1920.234193] Call trace: > <4>[ 1920.234211] [] __switch_to+0x80/0x8c > <4>[ 1920.234232] [] __schedule+0x550/0x7d8 > <4>[ 1920.234251] [] schedule+0x78/0x84 > <4>[ 1920.234268] [] schedule_timeout+0x40/0x2ac > <4>[ 1920.234285] [] wait_for_common+0x154/0x18c > <4>[ 1920.234301] [] wait_for_completion+0x24/0x34 > <4>[ 1920.234319] [] kthread_stop+0x130/0x22c > <4>[ 1920.234335] [] cpufreq_sched_setup+0x21c/0x308 > <4>[ 1920.234355] [] __cpufreq_governor+0x114/0x1c8 > <4>[ 1920.234375] [] cpufreq_set_policy+0x120/0x1b8 > <4>[ 1920.234395] [] store_scaling_governor+0x8c/0xd4 > <4>[ 1920.234413] [] store+0x98/0xd0 > <4>[ 1920.234432] [] sysfs_kf_write+0x54/0x64 > <4>[ 1920.234449] [] kernfs_fop_write+0x108/0x150 > <4>[ 1920.234470] [] vfs_write+0xc4/0x1a0 > <4>[ 1920.234489] [] SyS_write+0x60/0xb4 > > This happened while the kernel is switching from the sched governor to > the userspace governor. There's a race between kthread_stop() and > cpufreq_sched_thread(). On the previous version I was testing, I can > easily reproduce the lockup if I add a msleep(100) right before > set_current_state(TASK_INTERRUPTIBLE), and then switching between the > two governors through sysfs. Yes thanks for pointing this out. I've incorporated your fix, it will be part of the next RFC series I send out. thanks, Steve