From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965341AbcBDNBz (ORCPT ); Thu, 4 Feb 2016 08:01:55 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:31218 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbcBDNBy (ORCPT ); Thu, 4 Feb 2016 08:01:54 -0500 Subject: Re: [PATCH 09/54] perf tools: Add API to config maps in bpf object To: Arnaldo Carvalho de Melo References: <1453715801-7732-1-git-send-email-wangnan0@huawei.com> <1453715801-7732-10-git-send-email-wangnan0@huawei.com> <20160203232939.GB12194@redhat.com> CC: Alexei Starovoitov , Brendan Gregg , Daniel Borkmann , "David S. Miller" , He Kuang , Jiri Olsa , Li Zefan , Masami Hiramatsu , Namhyung Kim , Peter Zijlstra , , Will Deacon , From: "Wangnan (F)" Message-ID: <56B34B32.3010602@huawei.com> Date: Thu, 4 Feb 2016 20:59:30 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160203232939.GB12194@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.56B34B44.0145,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: afe249af6cc0d7b97f40e3b67cbc0547 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/2/4 7:29, Arnaldo Carvalho de Melo wrote: > Em Mon, Jan 25, 2016 at 09:55:56AM +0000, Wang Nan escreveu: [SNIP] >> + >> +static void >> +bpf_map_op__free(struct bpf_map_op *op) >> +{ >> + struct list_head *list = &op->list; >> + /* >> + * bpf_map_op__free() needs to consider following cases: >> + * 1. When the op is created but not linked to any list: >> + * impossible. This only happen in bpf_map_op__alloc() >> + * and it would be freed directly; >> + * 2. Normal case, when the op is linked to a list; >> + * 3. After the op has already be removed. >> + * Thanks to list.h, if it has removed by list_del() then >> + * list->{next,prev} should have been set to LIST_POISON{1,2}. >> + */ >> + if ((list->next != LIST_POISON1) && (list->prev != LIST_POISON2)) > Humm, this seems to rely on a debugging feature (setting something to a > trap value), i.e. list poisoning, shouldn't establish that removal needs > to be done via list_del_init() and then we would just check it with > list_empty(), which would be just like that bug we fixed recently wrt > thread__put(), the check, i.e. this is not problematic: > > list_del_init(&op->list); > list_del_init(&op->list); > > And after: > > list_del_init(&op->list); > > if you wanted for some reason to check if it was unlinked, this would do > the trick: > > if (!list_empty(&op->list) /* Is op in a list? */ > list_del_init(&op->list); > > static void bpf_map_op__free(struct bpf_map_op *op) > { > list_del(&op->list); /* Make sure it is removed */ > free(op); > } > > If we make sure that all list removal is done with list_del_init(). > > But then, this "make sure it is removed" looks strange, this should be > done only if it isn't linked, no? Perhaps use refcounts here? > > >> + list_del(list); >> + free(op); > > I.e. this function could be rewritten as: > >> +} >> + >> +static void >> +bpf_map_priv__clear(struct bpf_map *map __maybe_unused, >> + void *_priv) >> +{ >> + struct bpf_map_priv *priv = _priv; >> + struct bpf_map_op *pos, *n; >> + >> + list_for_each_entry_safe(pos, n, &priv->ops_list, list) >> + bpf_map_op__free(pos); > > I.e. here you would remove the thing and then call the delete() > operation for bpf_map_op, otherwise that delete(). > > Also normally this would be called bpf_map_priv__purge(), i.e. remove > entries and delete them, used in tools in: The name of bpf_map_priv__clear comes from bpf_map_clear_priv_t. It would be passed to bpf_map__set_private as a callback. Naming it using bpf_map_priv__purge() whould be confusing. I'll try another way to make things clear. Please see next version. Thank you.