Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753312AbZFSExT (ORCPT ); Fri, 19 Jun 2009 00:53:19 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751279AbZFSExK (ORCPT ); Fri, 19 Jun 2009 00:53:10 -0400 Received: from mail-gx0-f214.google.com ([209.85.217.214]:53495 "EHLO mail-gx0-f214.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751199AbZFSExJ (ORCPT ); Fri, 19 Jun 2009 00:53:09 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=h0rYTtiEnyZnDRPfky9ZraHX55Dm2vOCKPeBLWb1lQTFMyRtoY5ynoErByeGQCLC7v CoCgw3wMHtGeMXL5TUsUG+5o4pw2BQPhLJHzJewhLr4Xcu/aODj6/Frh1frPTVl8xOBw 2sZk8ge3wJ7ifDI3SNgV29XkR1tvhGfiRjBNQ= Subject: Re: [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable From: Tom Zanussi To: Steven Rostedt Cc: Li Zefan , Ingo Molnar , Frederic Weisbecker , LKML In-Reply-To: References: <4A38AD7F.2070408@cn.fujitsu.com> <1245304705.6207.78.camel@tropicana> Content-Type: text/plain Date: Thu, 18 Jun 2009 23:46:42 -0500 Message-Id: <1245386803.9918.49.camel@tropicana> Mime-Version: 1.0 X-Mailer: Evolution 2.24.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3981 Lines: 96 On Thu, 2009-06-18 at 19:18 -0400, Steven Rostedt wrote: > > > On Thu, 18 Jun 2009, Tom Zanussi wrote: > > > Hi, > > > > On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote: > > > | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 > > > | Author: Tom Zanussi > > > | Date: Sun Mar 22 03:31:04 2009 -0500 > > > | > > > | tracing: add per-event filtering > > > | ... > > > | > > > | Filters can also be set or cleared for a complete subsystem by writing > > > | the same filter as would be written to an individual event to the > > > | filter file at the root of the subsytem. Note however, that if any > > > | event in the subsystem lacks a field specified in the filter being > > > | set, the set will fail and all filters in the subsytem are > > > | automatically cleared. This change from the previous version was made > > > | because using only the fields that happen to exist for a given event > > > | would most likely result in a meaningless filter. > > > > > > I really don't like this change. It is inconvenient, and makes subsystem > > > filter much less useful: > > > > > > # echo 'vec == 1' > irq/softirq_entry/filter > > > # echo 'irq == 5' > irq/filter > > > bash: echo: write error: Invalid argument > > > # cat irq/softirq_entry/filter > > > none > > > > > > I'd expect this will set the filter for irq_handler_entry and > > > irq_handler_exit, and won't touch softirq_entry and softirq_exit. > > > > > > But it just failed, and what's worse, each event's filter was > > > cleared. > > > > > > > The idea behind the change was that after setting a subsystem filter, > > you'd be guaranteed that all or none of the events in the subsystem > > would have the same filter at that point, and not some mix of different > > filters depending on which ones failed or not, which to me seemed > > nonintuitive. > > > > If I set a filter like "vec == 1 && irq == 5", which really has no > > overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and > > irq_handler_entry to get "irq == 5" - I'd rather get an error, but > > that's just me. > > > > So if it makes more sense to users to have subsystem filters propagate > > to whichever events will take them, this patch would be fine with me. > > > > I disagree. If a subsystem filter is set, it should be valid for all > filters underneath, if it is not, it should error. > > But Li has a point, if you get an error, it should not reset all filters > underneath. That is, if the irq/filter setting took an error, then > irq/softirq_entry/filter should still stay the same. > > Perhaps you need to run through it twice. See if the setting of a filter > is valid for all filters underneath, if it is not, then fail. If it is, > then reset all of them, and assign the filter. > Yeah, I agree that this is better than just clearing them all on an error, but it still means that a subsystem filter will succeed only when it names common_ (or commonly named) fields. I think what Li is saying is that that restriction makes the subsystem filters less useful, and you should for convenience' sake be allowed to propagate a filter to a subset and ignore the ones that don't make sense. Note that there's a danger in this case that a filter might be applied but not really make sense e.g. two events might have a 'vec' field that mean completely different things but the filter would be applied to both just because they have the same name. The only way to ensure that they would always make sense would be to restrict the subsystem filter to just the common_ fields. But that's less useful, and maybe it would be better to leave the choice up the user... Tom > -- Steve > -- 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/