Re: [PATCH bpf-next v4 2/2] perf: stop using deprecated bpf_object__next() API

[Jiri Olsa <jolsa@redhat.com>]

hi,
just checking, do you plan to send new version for this?

thanks,
jirka

On Sun, Jan 23, 2022 at 06:06:08PM +0100, Jiri Olsa wrote:
> On Wed, Jan 19, 2022 at 03:06:36PM -0800, Christy Lee wrote:
> 
> SNIP
> 
> > ---
> >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> >  tools/perf/util/bpf-loader.h |  1 +
> >  2 files changed, 55 insertions(+), 18 deletions(-)
> > 
> > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > index 4631cac3957f..b1822f8af2bb 100644
> > --- a/tools/perf/util/bpf-loader.c
> > +++ b/tools/perf/util/bpf-loader.c
> > @@ -29,9 +29,6 @@
> >  
> >  #include <internal/xyarray.h>
> >  
> > -/* temporarily disable libbpf deprecation warnings */
> > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > -
> >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> >  			      const char *fmt, va_list args)
> >  {
> > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> >  	int *type_mapping;
> >  };
> >  
> > +struct bpf_perf_object {
> > +	struct bpf_object *obj;
> > +	struct list_head list;
> > +};
> > +
> > +static LIST_HEAD(bpf_objects_list);
> 
> hum.. I still can't see any code adding/removing bpf_perf_object
> objects to this list, and that's why the code is failing to remove
> probes
> 
> because there are no objects to iterate on, so added probes stay
> configured and screw following tests
> 
> you need something like below to add and del objects from
> bpf_objects_list list
> 
> it also simplifies for_each macros to work just over perf_obj,
> because I wasn't patient enough to make it work with the extra
> bpf_object ;-) I don't mind if you fix that, but this way looks
> simpler to me
> 
> tests are working for me with this fix, please feel free to
> squash it into your change
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
> index 57b9591f7cbb..d09d25707f1e 100644
> --- a/tools/perf/tests/bpf.c
> +++ b/tools/perf/tests/bpf.c
> @@ -210,6 +210,11 @@ prepare_bpf(void *obj_buf, size_t obj_buf_sz, const char *name)
>  		pr_debug("Compile BPF program failed.\n");
>  		return NULL;
>  	}
> +
> +	if (bpf_perf_object__add(obj)) {
> +		bpf_object__close(obj);
> +		return NULL;
> +	}
>  	return obj;
>  }
>  
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 89e584ac267c..a7a8ad32c847 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -70,11 +70,11 @@ struct bpf_perf_object *bpf_perf_object__next(struct bpf_perf_object *prev)
>  	return next;
>  }
>  
> -#define bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> -	for ((perf_obj) = bpf_perf_object__next(NULL),                         \
> -	    (tmp) = bpf_perf_object__next(perf_obj), (obj) = NULL;             \
> -	     (perf_obj) != NULL; (perf_obj) = (tmp),                           \
> -	    (tmp) = bpf_perf_object__next(tmp), (obj) = (perf_obj)->obj)
> +#define bpf_perf_object__for_each(perf_obj, tmp)         \
> +	for ((perf_obj) = bpf_perf_object__next(NULL),   \
> +	     (tmp) = bpf_perf_object__next(perf_obj);    \
> +	     (perf_obj) != NULL; (perf_obj) = (tmp),     \
> +	    (tmp) = bpf_perf_object__next(tmp) )
>  
>  static bool libbpf_initialized;
>  
> @@ -97,6 +97,24 @@ bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
>  	return obj;
>  }
>  
> +int bpf_perf_object__add(struct bpf_object *obj)
> +{
> +	struct bpf_perf_object *perf_obj = zalloc(sizeof(*perf_obj));
> +
> +	if (perf_obj) {
> +		perf_obj->obj = obj;
> +		list_add_tail(&perf_obj->list, &bpf_objects_list);
> +	}
> +	return perf_obj ? 0 : -ENOMEM;
> +}
> +
> +static void bpf_perf_object__close(struct bpf_perf_object *perf_obj)
> +{
> +	list_del(&perf_obj->list);
> +	bpf_object__close(perf_obj->obj);
> +	free(perf_obj);
> +}
> +
>  struct bpf_object *bpf__prepare_load(const char *filename, bool source)
>  {
>  	struct bpf_object *obj;
> @@ -135,17 +153,20 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
>  		return obj;
>  	}
>  
> +	if (bpf_perf_object__add(obj)) {
> +		bpf_object__close(obj);
> +		return ERR_PTR(-BPF_LOADER_ERRNO__COMPILE);
> +	}
>  	return obj;
>  }
>  
>  void bpf__clear(void)
>  {
>  	struct bpf_perf_object *perf_obj, *tmp;
> -	struct bpf_object *obj;
>  
> -	bpf_perf_object__for_each(perf_obj, tmp, obj) {
> -		bpf__unprobe(obj);
> -		bpf_object__close(obj);
> +	bpf_perf_object__for_each(perf_obj, tmp) {
> +		bpf__unprobe(perf_obj->obj);
> +		bpf_perf_object__close(perf_obj);
>  	}
>  }
>  
> @@ -1538,11 +1559,10 @@ apply_obj_config_object(struct bpf_object *obj)
>  int bpf__apply_obj_config(void)
>  {
>  	struct bpf_perf_object *perf_obj, *tmp;
> -	struct bpf_object *obj;
>  	int err;
>  
> -	bpf_perf_object__for_each(perf_obj, tmp, obj) {
> -		err = apply_obj_config_object(obj);
> +	bpf_perf_object__for_each(perf_obj, tmp) {
> +		err = apply_obj_config_object(perf_obj->obj);
>  		if (err)
>  			return err;
>  	}
> @@ -1550,25 +1570,24 @@ int bpf__apply_obj_config(void)
>  	return 0;
>  }
>  
> -#define bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> -	bpf_perf_object__for_each(perf_obj, tmp, obj)                          \
> -		bpf_object__for_each_map(map, obj)
> +#define bpf__perf_for_each_map(perf_obj, tmp, map)                   \
> +	bpf_perf_object__for_each(perf_obj, tmp)                     \
> +		bpf_object__for_each_map(map, perf_obj->obj)
>  
> -#define bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name)            \
> -	bpf__perf_for_each_map(perf_obj, tmp, obj, map)                        \
> +#define bpf__perf_for_each_map_named(perf_obj, tmp, map, name)            \
> +	bpf__perf_for_each_map(perf_obj, tmp, map)                        \
>  		if (bpf_map__name(map) && (strcmp(name, bpf_map__name(map)) == 0))
>  
>  struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
>  {
>  	struct bpf_map_priv *tmpl_priv = NULL;
>  	struct bpf_perf_object *perf_obj, *tmp;
> -	struct bpf_object *obj;
>  	struct evsel *evsel = NULL;
>  	struct bpf_map *map;
>  	int err;
>  	bool need_init = false;
>  
> -	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
> +	bpf__perf_for_each_map_named(perf_obj, tmp, map, name) {
>  		struct bpf_map_priv *priv = bpf_map__priv(map);
>  
>  		if (IS_ERR(priv))
> @@ -1604,7 +1623,7 @@ struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name)
>  		evsel = evlist__last(evlist);
>  	}
>  
> -	bpf__perf_for_each_map_named(perf_obj, tmp, obj, map, name) {
> +	bpf__perf_for_each_map_named(perf_obj, tmp, map, name) {
>  		struct bpf_map_priv *priv = bpf_map__priv(map);
>  
>  		if (IS_ERR(priv))
> diff --git a/tools/perf/util/bpf-loader.h b/tools/perf/util/bpf-loader.h
> index 95262b7e936f..78c7d3662910 100644
> --- a/tools/perf/util/bpf-loader.h
> +++ b/tools/perf/util/bpf-loader.h
> @@ -83,6 +83,8 @@ int bpf__strerror_config_obj(struct bpf_object *obj,
>  int bpf__apply_obj_config(void);
>  int bpf__strerror_apply_obj_config(int err, char *buf, size_t size);
>  
> +int bpf_perf_object__add(struct bpf_object *obj);
> +
>  int bpf__setup_stdout(struct evlist *evlist);
>  struct evsel *bpf__setup_output_event(struct evlist *evlist, const char *name);
>  int bpf__strerror_setup_output_event(struct evlist *evlist, int err, char *buf, size_t size);