Re: [PATCH bpf-next v3 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Feb 05, 2024 at 12:45:30PM -0700, Daniel Xu wrote:

SNIP

> > @@ -654,10 +655,10 @@ static int sets_patch(struct object *obj)
> >  
> >  	next = rb_first(&obj->sets);
> >  	while (next) {
> > +		struct btf_id_set8 *set8;
> > +		struct btf_id_set *set;
> >  		unsigned long addr, idx;
> >  		struct btf_id *id;
> > -		int *base;
> > -		int cnt;
> >  
> >  		id   = rb_entry(next, struct btf_id, rb_node);
> >  		addr = id->addr[0];
> > @@ -671,13 +672,21 @@ static int sets_patch(struct object *obj)
> >  		}
> >  
> >  		idx = idx / sizeof(int);
> > -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> > -		cnt = ptr[idx];
> > +		if (id->is_set) {
> > +			set = (struct btf_id_set *)&ptr[idx];
> 
> Nit: should be able to simplify logic a bit like this:
> 
>         int off = addr - obj->efile.idlist_addr;
>         set8 = data->d_buf + off;
> 
> Don't think that `idx`, `ptr` or casts are necessary anymore.

+1 , otherwise it looks good to me

jirka

> 
> > +			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
> > +		} else {
> > +			set8 = (struct btf_id_set8 *)&ptr[idx];
> > +			/*
> > +			 * Make sure id is at the beginning of the pairs
> > +			 * struct, otherwise the below qsort would not work.
> > +			 */
> > +			BUILD_BUG_ON(set8->pairs != &set8->pairs[0].id);
> > +			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
> > +		}
> >  
> >  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> > -			 (idx + 1) * sizeof(int), cnt, id->name);
> > -
> > -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> > +			 (idx + 1) * sizeof(int), id->is_set ? set->cnt : set8->cnt, id->name);
> >  
> >  		next = rb_next(next);
> >  	}
> > diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> > index 2f882d5cb30f..72535f00572f 100644
> > --- a/tools/include/linux/btf_ids.h
> > +++ b/tools/include/linux/btf_ids.h
> > @@ -8,6 +8,15 @@ struct btf_id_set {
> >  	u32 ids[];
> >  };
> >  
> > +struct btf_id_set8 {
> > +	u32 cnt;
> > +	u32 flags;
> > +	struct {
> > +		u32 id;
> > +		u32 flags;
> > +	} pairs[];
> > +};
> > +
> >  #ifdef CONFIG_DEBUG_INFO_BTF
> >  
> >  #include <linux/compiler.h> /* for __PASTE */
> > -- 
> > 2.43.0
> >