Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > +++ b/tools/perf/util/comm.c
> > @@ -18,11 +18,9 @@ struct comm_str {
> >  static struct rb_root comm_str_root;
> >  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
> >  
> > -static struct comm_str *comm_str__get(struct comm_str *cs)
> > +static bool comm_str__get(struct comm_str *cs)
> >  {
> > -	if (cs)
> > -		refcount_inc(&cs->refcnt);
> > -	return cs;
> > +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
> >  }
> 
> I don't like changing the semantics of a __get() operation this way, I
> think it should stay like all the others, i.e. return the object with
> the desired refcount or return NULL if that is not possible.
> 
> Otherwise we'll have to switch gears when debugging refcounts in various
> objects, that start having slightly different semantics for reference
> counting.
> 
> We should try to find a fix that maintains the semantics of refcounting.

After looking at the code, this refcount_inc_not_zero returns bool comes
from the kernel, trying to see how this is used with __get() operations
there, if at all.

- Arnaldo