From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755002AbbIBOoA (ORCPT ); Wed, 2 Sep 2015 10:44:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51952 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754376AbbIBOn6 (ORCPT ); Wed, 2 Sep 2015 10:43:58 -0400 Date: Wed, 2 Sep 2015 11:43:50 -0300 From: Arnaldo Carvalho de Melo To: pi3orama Cc: Jiri Olsa , "Wangnan (F)" , "masami.hiramatsu.pt@hitachi.com" , Alexei Starovoitov , Jiri Olsa , Namhyung Kim , Zefan Li , acme@kernel.org, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel Message-ID: <20150902144350.GK22331@redhat.com> References: <50399556C9727B4D88A595C8584AAB37525027C2@GSjpTKYDCembx32.service.hitachi.net> <1441176553-116129-1-git-send-email-wangnan0@huawei.com> <55E69D06.4050005@huawei.com> <20150902115415.GH8598@krava.brq.redhat.com> <20150902135513.GJ22331@redhat.com> <5AD321FB-E032-4E61-828D-17FE3998A46F@163.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5AD321FB-E032-4E61-828D-17FE3998A46F@163.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.20 (2009-12-10) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Wed, Sep 02, 2015 at 10:04:21PM +0800, pi3orama escreveu: > 发自我的 iPhone > > 在 2015年9月2日，下午9:55，Arnaldo Carvalho de Melo 写道： > > Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu: > >> 发自我的 iPhone > >>> 在 2015年9月2日，下午7:54，Jiri Olsa 写道： > >>>>> On Wed, Sep 02, 2015 at 02:53:58PM +0800, Wangnan (F) wrote: > >>>>> @@ -1252,7 +1262,13 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist, > >>>>> struct perf_evsel *last = NULL; > >>>>> int err; > >>>>> - if (evlist->nr_entries > 0) > >>>>> + /* > >>>>> + * Don't return when list_empty, give func a chance to report > >>>>> + * error when it found last == NULL. > >>>>> + * > >>>>> + * So no need to WARN here, let *func do this. > >>>>> + */ > >>>>> + if (!list_empty(&evlist->entries)) > >>> why is it better than to check evlist->nr_entries? > >>> evlist->nr_entries is equivalent to !list_empty(&evlist->entries) in here, right? > >> By checking list we won't rely on the assumption that nr_entries reflects the > >> actual number of elements in that list, makes the logic of this code more compact. > > But why would we want to break that assumption? > > If I see FOO->entries and FOO->nr_entries, it is reasonable to expect > > that whatever data structure FOO->entries may be has FOO->nr_entries in > > it, lets not break that assumption. > Then we should enforce it. Agreed, but it is a reasonable expectation, right? Its a general pattern, one that we expect and when it breaks like that, that may lead to bugs :-) > For example, check the list collected by parser, report an error if > the list is empty, to avoid someone like me adding nothing on the list > but report success. I'm not insistent on this patch. In my newest > patch set I use real dummy evsel as placeholder so we won't meet empty > list again. Ok, I'll look at the new patch then, I keep thinking that if you need to have a separate list for eBPF, that you will do something special on it, etc, then that is not a problem just keep it as a separate list till you can insert it in the evlist to then open the evlist, mmap it, etc. If in the parsing routines you have access only to a perf_evlist pointer, well, then we can have something like an evlist->pending_entries + evlist->nr_pending_entries. Something like that. If you have detailed why you need it to be left in the evlist (will some operation be done on all evsels, even the ones that need eBPF specific work before you do this final eBPF specific stuff?), I'll try to find it, if not, describing the sequence of events that justifies this or the "dummy evsel as a placeholder" would help reviewing this, treat us like 7 year old kids (aka use patience + details) :-) - Arnaldo