Re: [PATCH v2 06/13] tcg: call qemu_spin_destroy for tb->jmp_lock

[Robert Foley]

On Mon, 8 Jun 2020 at 10:44, Alex Bennée <alex.bennee@linaro.org> wrote:
<snip>
> > +static void tcg_region_tree_reset_all(tb_destroy_func tb_destroy)
> >  {
> >      size_t i;
> >
> > @@ -510,6 +519,10 @@ static void tcg_region_tree_reset_all(void)
> >      for (i = 0; i < region.n; i++) {
> >          struct tcg_region_tree *rt = region_trees + i * tree_size;
> >
> > +        if (tb_destroy != NULL) {
> > +            g_tree_foreach(rt->tree, tcg_region_tree_traverse, tb_destroy);
> > +        }
> > +
>
> Isn't tb_destroy always set? We could assert that is the case rather
> than make the cleaning up conditional.

I agree, tb_destroy seems to always be set, so the assert would be reasonable.

>
> >          /* Increment the refcount first so that destroy acts as a reset */
> >          g_tree_ref(rt->tree);
> >          g_tree_destroy(rt->tree);
> > @@ -586,7 +599,7 @@ static inline bool 
> > tcg_region_initial_alloc__locked(TCGContext *s)
> >  }
> >
> >  /* Call from a safe-work context */
> > -void tcg_region_reset_all(void)
> > +void tcg_region_reset_all(tb_destroy_func tb_destroy)
> >  {
> >      unsigned int n_ctxs = atomic_read(&n_tcg_ctxs);
> >      unsigned int i;
> > @@ -603,7 +616,7 @@ void tcg_region_reset_all(void)
> >      }
> >      qemu_mutex_unlock(&region.lock);
> >
> > -    tcg_region_tree_reset_all();
> > +    tcg_region_tree_reset_all(tb_destroy);
>
> Could you name the variables of type tb_destroy_func differently as
> although the variable is only ever tb_destroy the function it gets
> confusing real quick when trying to grep for stuff. Maybe tbd_fn?
>
> That said given the single usage why a function pointer? Would we be
> just as well served by an exposed public function call from the
> appropriate places?

Good point.  Unless we see an imminent need to pass different values,
then it seems
reasonable to just use the public function call and remove the need for
the function pointer.

Thanks & Regards,
-Rob


>
> Richard do you have a view here?
>
> --
> Alex Bennée