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.
