Re: [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jan 03, 2025 at 12:43:42AM +0000, Ihor Solodrai wrote:
> On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> > 
> > On Sat, Dec 21, 2024 at 01:23:45AM +0000, Ihor Solodrai wrote:
> > 
> > SNIP
> > 
> > > -static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > > +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
> > > {
> > > struct btf_encoder_func_state **saved_fns, *s;
> > > - struct btf_encoder *e = NULL;
> > > - int i = 0, j, nr_saved_fns = 0;
> > > + int err = 0, i = 0, j, nr_saved_fns = 0;
> > > 
> > > - /* Retrieve function states from each encoder, combine them
> > > + /* Retrieve function states from the encoder, combine them
> > > * and sort by name, addr.
> > > */
> > > - btf_encoders__for_each_encoder(e) {
> > > - list_for_each_entry(s, &e->func_states, node)
> > > - nr_saved_fns++;
> > > + list_for_each_entry(s, &encoder->func_states, node) {
> > > + nr_saved_fns++;
> > > }
> > > 
> > > if (nr_saved_fns == 0)
> > > - return 0;
> > > + goto out;
> > > 
> > > saved_fns = calloc(nr_saved_fns, sizeof(*saved_fns));
> > > - btf_encoders__for_each_encoder(e) {
> > > - list_for_each_entry(s, &e->func_states, node)
> > > - saved_fns[i++] = s;
> > > + if (!saved_fns) {
> > > + err = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + list_for_each_entry(s, &encoder->func_states, node) {
> > > + saved_fns[i++] = s;
> > > }
> > > qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
> > > 
> > > @@ -1377,11 +1313,10 @@ static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > > 
> > > /* Now that we are done with function states, free them. */
> > > free(saved_fns);
> > > - btf_encoders__for_each_encoder(e) {
> > > - btf_encoder__delete_saved_funcs(e);
> > > - }
> > > + btf_encoder__delete_saved_funcs(encoder);
> > 
> > 
> > is this call necessary? there's btf_encoder__delete call right after
> > same for elf_functions_list__clear in btf_encoder__encode
> 
> At this point we know that the function information is no longer
> needed, so we can free up some memory.
> 
> I remember when I wrote a "context" patch [1] (now discarded), there
> was a significant difference in MAX RSS between freeing this right
> away and delaying until encoding is finished. Now it might not be as
> significant, but still there is no reason to keep the stuff we know is
> not used later.

ok, makes sense

jirka

> 
> [1] https://lore.kernel.org/dwarves/20241213223641.564002-8-ihor.solodrai@pm.me/
> 
> > 
> > thanks,
> > jirka
> > 
> > > - return 0;
> > > +out:
> > > + return err;
> > > }
> > 
> > 
> > SNIP
>