Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966499Ab2ERSxT (ORCPT ); Fri, 18 May 2012 14:53:19 -0400 Received: from s15943758.onlinehome-server.info ([217.160.130.188]:42023 "EHLO mail.x86-64.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964842Ab2ERSxQ (ORCPT ); Fri, 18 May 2012 14:53:16 -0400 Date: Fri, 18 May 2012 20:52:58 +0200 From: Borislav Petkov To: Mauro Carvalho Chehab Cc: Borislav Petkov , Ingo Molnar , Linux Edac Mailing List , Linux Kernel Mailing List , Aristeu Rozanski , Doug Thompson , Steven Rostedt , Frederic Weisbecker , Ingo Molnar Subject: Re: [PATCH v24b] RAS: Add a tracepoint for reporting memory controller events Message-ID: <20120518185258.GE20215@aftab.osrc.amd.com> References: <20120517214859.GA16777@aftab.osrc.amd.com> <20120518071244.GE429@gmail.com> <20120518095638.GA20215@aftab.osrc.amd.com> <4FB62BAB.90808@redhat.com> <20120518124338.GJ20215@aftab.osrc.amd.com> <4FB64D5B.1030301@redhat.com> <20120518140521.GM20215@aftab.osrc.amd.com> <4FB65D52.9060108@redhat.com> <20120518164014.GX20215@aftab.osrc.amd.com> <4FB6866E.7090503@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4FB6866E.7090503@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5007 Lines: 145 On Fri, May 18, 2012 at 02:27:10PM -0300, Mauro Carvalho Chehab wrote: > Em 18-05-2012 13:40, Borislav Petkov escreveu: > > On Fri, May 18, 2012 at 11:31:46AM -0300, Mauro Carvalho Chehab wrote: > >> Ok, but you won't use trace_sched_switch() as a memory tracepoint, as > >> they represent different things. > >> > >> Memory errors are different than CPU errors. So, their tracepoints > >> will be different. > > > > WTF? A tracepoint is a tracepoint. > > This is the event you've mentioned: > > TRACE_EVENT(sched_switch, > > TP_PROTO(struct task_struct *prev, > struct task_struct *next), > > It doesn't have the same arguments of a memory tracepoint, like: > > TRACE_EVENT(mm_vmscan_wakeup_kswapd, > > TP_PROTO(int nid, int zid, int order), > > trace events for different things will have different arguments. It seems you and I speak two different english languages. Here's what I meant: diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 0533a688ce22..96947fb50328 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1918,6 +1918,7 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev, prepare_lock_switch(rq, next); prepare_arch_switch(next); trace_sched_switch(prev, next); + trace_sched_switch_rq(prev, next, rq); } because for some reason we want to dump something from the runqueue pointer now. We can't change the current tracepoint because userspace scripts use it. Now imagine the bloat levels when people start adding tracepoints left and right... > > > >>> Right, and this is why I'm asking you to have the following tracepoint proto: > >>> > >>> + TP_PROTO(const unsigned int err_type, > >>> + const unsigned int mc_index, > >>> + const char *error_msg, > >>> + const char *label, > >>> + const char *location, > >>> + const char *detail) > >>> > >>> where detail contains all the crap one driver adds for technical people > >>> to pinpoint where the error is. > >>> > >>> And not have _TWO_ detail arguments! > >> > >> And what I'm saying is that this should be, instead: > >> > >> + TP_PROTO(const unsigned int err_type, > >> + const unsigned int mc_index, > >> + const char *error_msg, > >> + const char *label, > >> + int layer0, > >> + int layer1, > >> + int layer2, > >> + unsigned long pfn, > >> + unsigned long offset, > >> + unsigned long grain, > >> + unsigned long syndrome, > >> + const char *driver_detail), > >> > >> So, having just one detail argument, filled by the driver, and not > >> folding "location" and core "details" into strings, but keeping as they > >> are. > > > > And this way you're enforcing an interface that all drivers will have > > to adhere to. What if "grain" doesn't mean a thing for a driver, or > > "syndrome" or whatever? What if some other entity wants to use that > > tracepoint? > > This enforcement is already there at the EDAC API/ABI. This patch series > won't change that. > > Memory controllers that can't get the error address (page/offset/grain) > or the syndrome fills those information with 0. Setting something to 0 because it is N/A is the first sign that something is wrong with your ABI. Having an arbitrary string which each driver parses as it sees fit is perfectly fine. > > See what I'm sayin? > > > > Having > > > > TP_PROTO(const unsigned int err_type, > > const unsigned int mc_index, > > const char *error_msg, > > const char *label, > > const char *location, > > const char *detail) > > > > is a bit more generic and userspace can parse it however it likes. > > > > Actually, I'd slim this up even more: > > > > TP_PROTO(const unsigned int mc_index, > > const char *error_msg, > > const char *label, > > const char *location, > > const char *detail) > > Well, this even more generic: > TP_PROTO(const char *msg) > > but the more we convert other fields into strings, the harder is for > userspace to handle it, For the millionth time, it is not hard for userspace to parse anything! > as fields may be on different order, string conversion patches might > change or break the parsing behavior, different drivers will require > different parsing expressions, etc. That's why _each_ _driver_ will have its format and the userspace tools parsing it will know about it! -- Regards/Gruss, Boris. Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach GM: Alberto Bozzo Reg: Dornach, Landkreis Muenchen HRB Nr. 43632 WEEE Registernr: 129 19551 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/