Re: [PATCH 09/54] perf tools: Add API to config maps in bpf object

["Wangnan (F)" <wangnan0@huawei.com>]

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.