From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965351AbcAUPvr (ORCPT ); Thu, 21 Jan 2016 10:51:47 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:42980 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759312AbcAUPvo (ORCPT ); Thu, 21 Jan 2016 10:51:44 -0500 Date: Thu, 21 Jan 2016 16:51:20 +0100 From: Peter Zijlstra To: Huang Rui Cc: Borislav Petkov , Ingo Molnar , Andy Lutomirski , Thomas Gleixner , Robert Richter , Jacob Shin , John Stultz , =?utf-8?B?RnLvv71k77+9cmlj?= Weisbecker , linux-kernel@vger.kernel.org, spg_linux_kernel@amd.com, x86@kernel.org, Guenter Roeck , Andreas Herrmann , Suravee Suthikulpanit , Aravind Gopalakrishnan , Borislav Petkov , Fengguang Wu , Aaron Lu Subject: Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power reporting mechanism Message-ID: <20160121155120.GI6357@twins.programming.kicks-ass.net> References: <1452739808-11871-1-git-send-email-ray.huang@amd.com> <1452739808-11871-6-git-send-email-ray.huang@amd.com> <20160119121250.GA6344@twins.programming.kicks-ass.net> <20160120044823.GA13477@hr-amur2> <20160120092244.GH6357@twins.programming.kicks-ass.net> <20160121070437.GA15130@hr-amur2> <20160121090257.GC6357@twins.programming.kicks-ass.net> <20160121144233.GA16294@hr-amur2> <20160121151040.GO6356@twins.programming.kicks-ass.net> <20160121152412.GB16294@hr-amur2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160121152412.GB16294@hr-amur2> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 21, 2016 at 11:24:14PM +0800, Huang Rui wrote: > On Thu, Jan 21, 2016 at 04:10:40PM +0100, Peter Zijlstra wrote: > > On Thu, Jan 21, 2016 at 10:42:35PM +0800, Huang Rui wrote: > > > > > @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event *event, int mode) > > > > > struct hw_perf_event *hwc = &event->hw; > > > > > unsigned long flags; > > > > > > > > > > - spin_lock_irqsave(&pmu->lock, flags); > > > > > + raw_spin_lock_irqsave(&pmu->lock, flags); > > > > > > > > > > hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED; > > > > > > > > > > if (mode & PERF_EF_START) > > > > > __pmu_event_start(pmu, event); > > > > > > > > > > - spin_unlock_irqrestore(&pmu->lock, flags); > > > > > + raw_spin_unlock_irqrestore(&pmu->lock, flags); > > > > > > > > > > return 0; > > > > > } > > > > > > > > So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore > > > > thing as its guaranteed that IRQs will be disabled. > > > > > > > > > > OK, I will remove the lock. > > > > No, the lock seems needed, as the list is global. Just the > > irqsave/irqrestore part is superfluous. > > > > But actually, the lock is only used at {start,stop,add,del}. If we > drop irqsave/irqrestore on these 4 things, there won't be any use > cases. But, but, the list is global !? something needs to serialize the access to it, right?