Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S933411AbbGGTsU (ORCPT <rfc822;w@1wt.eu>);
	Tue, 7 Jul 2015 15:48:20 -0400
Received: from mail.kernel.org ([198.145.29.136]:46358 "EHLO mail.kernel.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932636AbbGGTsF (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 7 Jul 2015 15:48:05 -0400
Date: Tue, 7 Jul 2015 16:47:59 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Wang Nan <wangnan0@huawei.com>
Cc: ast@plumgrid.com, brendan.d.gregg@gmail.com, daniel@iogearbox.net,
        namhyung@kernel.org, masami.hiramatsu.pt@hitachi.com, paulus@samba.org,
        a.p.zijlstra@chello.nl, mingo@redhat.com, jolsa@kernel.org,
        dsahern@gmail.com, linux-kernel@vger.kernel.org, lizefan@huawei.com,
        hekuang@huawei.com, xiakaixu@huawei.com, pi3orama@163.com
Subject: Re: [RFC PATCH v10 22/50] bpf tools: Link all bpf objects onto a list
Message-ID: <20150707194759.GC3135@kernel.org>
References: <1435716878-189507-1-git-send-email-wangnan0@huawei.com>
 <1435716878-189507-23-git-send-email-wangnan0@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1435716878-189507-23-git-send-email-wangnan0@huawei.com>
X-Url: http://acmel.wordpress.com
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4199
Lines: 136

Em Wed, Jul 01, 2015 at 02:14:10AM +0000, Wang Nan escreveu:
> To prevent caller from creating additional structures to hold
> pointers of 'struct bpf_object', this patch link all such

Can you rephrase the above sentence? You mean "to allow enumeration of
all bpf_objects, keep them in a list".

> structures onto a list (hidden to user). bpf_object__for_each() is
> introduced to allow users iterate over each objects.
> bpf_object__for_each() is safe even user close the object during
> iteration.

We have a idiom for that, it s called foo__for_each_entry_safe(), see
list.h, tools/perf/util/evlist.h as well usess those same idioms, please
keep that semantic.

I.e. if one isn't modifying the list while traversing it, no need for
that tmp, if one is, the _safe() variant should be used, that requires
the tmp.

We also have list_first_entry(), list_last_entry(), etc, its good to
keep those semantics, helps people used to kernel (and tools/)
programming to feel at home when reading your code :-)

- Arnaldo
 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
>  tools/lib/bpf/libbpf.c | 32 ++++++++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.h |  7 +++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1c210fb..540ac558 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -17,6 +17,7 @@
>  #include <asm/unistd.h>
>  #include <linux/kernel.h>
>  #include <linux/bpf.h>
> +#include <linux/list.h>
>  #include <libelf.h>
>  #include <gelf.h>
>  
> @@ -104,6 +105,8 @@ struct bpf_program {
>  	bpf_program_clear_priv_t clear_priv;
>  };
>  
> +static LIST_HEAD(bpf_objects_list);
> +
>  struct bpf_object {
>  	char license[64];
>  	u32 kern_version;
> @@ -137,6 +140,12 @@ struct bpf_object {
>  		} *reloc;
>  		int nr_reloc;
>  	} efile;
> +	/*
> +	 * All loaded bpf_object is linked in a list, which is
> +	 * hidden to caller. bpf_objects__<func> handlers deal with
> +	 * all objects.
> +	 */
> +	struct list_head list;
>  	char path[];
>  };
>  #define obj_elf_valid(o)	((o)->efile.elf)
> @@ -254,6 +263,9 @@ static struct bpf_object *bpf_object__new(const char *path,
>  	obj->efile.obj_buf_sz = obj_buf_sz;
>  
>  	obj->loaded = false;
> +
> +	INIT_LIST_HEAD(&obj->list);
> +	list_add(&obj->list, &bpf_objects_list);
>  	return obj;
>  }
>  
> @@ -933,6 +945,7 @@ void bpf_object__close(struct bpf_object *obj)
>  	}
>  	zfree(&obj->programs);
>  
> +	list_del(&obj->list);
>  	free(obj);
>  }
>  
> @@ -945,6 +958,25 @@ int bpf_object__get_prog_cnt(struct bpf_object *obj, size_t *pcnt)
>  	return 0;
>  }
>  
> +struct bpf_object *
> +bpf_object__next(struct bpf_object *prev)
> +{
> +	struct bpf_object *next;
> +
> +	if (!prev)
> +		next = list_first_entry(&bpf_objects_list,
> +					struct bpf_object,
> +					list);
> +	else
> +		next = list_next_entry(prev, list);
> +
> +	/* Empty list is noticed here so don't need checking on entry. */
> +	if (&next->list == &bpf_objects_list)
> +		return NULL;
> +
> +	return next;
> +}
> +
>  struct bpf_program *
>  bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
>  {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index a20ae2e..1cd17a2 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -38,6 +38,13 @@ int bpf_object__unload(struct bpf_object *obj);
>  /* Accessors of bpf_object */
>  int bpf_object__get_prog_cnt(struct bpf_object *obj, size_t *pcnt);
>  
> +struct bpf_object *bpf_object__next(struct bpf_object *prev);
> +#define bpf_object__for_each(pos, tmp)			\
> +	for ((pos) = bpf_object__next(NULL),		\
> +		(tmp) = bpf_object__next(pos);		\
> +	     (pos) != NULL;				\
> +	     (pos) = (tmp), (tmp) = bpf_object__next(tmp))
> +
>  /* Accessors of bpf_program. */
>  struct bpf_program;
>  struct bpf_program *bpf_program__next(struct bpf_program *prog,
> -- 
> 1.8.3.4
--
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/