From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161658AbcFHIMB (ORCPT ); Wed, 8 Jun 2016 04:12:01 -0400 Received: from outbound-smtp09.blacknight.com ([46.22.139.14]:47169 "EHLO outbound-smtp09.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161585AbcFHILp (ORCPT ); Wed, 8 Jun 2016 04:11:45 -0400 Date: Wed, 8 Jun 2016 09:11:41 +0100 From: Mel Gorman To: Josh Poimboeuf Cc: Ingo Molnar , Peter Zijlstra , linux-kernel@vger.kernel.org, Matt Fleming , Srikar Dronamraju Subject: Re: [PATCH 2/2] sched/debug: fix deadlock when enabling sched events Message-ID: <20160608081141.GF2527@techsingularity.net> References: <50a0f7f2a5f8d69e0440c3430f06be7ccbe59903.1465322027.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <50a0f7f2a5f8d69e0440c3430f06be7ccbe59903.1465322027.git.jpoimboe@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 07, 2016 at 02:43:17PM -0500, Josh Poimboeuf wrote: > When enabling sched trace events via: > > echo 1 > /sys/kernel/debug/tracing/events/sched/enable > > I see a hang, with the following BUG in the printk buffer: > I was certain I had tested this case because I was monitoring to make sure the stats were not updating until the tuning knob was used. It was because the context was unsafe that any printing happened because at the time trying to enable stats in that context blew up. > In addition to the deadlock, I think this code has other issues: > > 1. Instead of just warning and allowing the tracepoints to be broken, > I'd argue that it would be better to make them work by forcing > schedstats enabled and printing a warning about that, which is what's > already being done in other similar cases (latencytop and profiling). > Otherwise things like "perf sched record" won't have the intended > effect. In fact the comment in the above code snippet seems to agree > with me, so maybe that was the original intent. > > 2. It's called in the scheduler hot path from enqueue_entity(). > > So change the warning to a call to force_schedstat_enabled(), and do it > only when the tracepoint gets enabled. > > Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default") > Cc: stable@vger.kernel.org > Signed-off-by: Josh Poimboeuf I like it! I wasn't aware there was an option to have a registration callback. Acked-by: Mel Gorman -- Mel Gorman SUSE Labs