Messages in this thread | | | Date | Fri, 20 Jul 2018 10:20:55 +0900 | From | Namhyung Kim <> | Subject | Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash |
| |
Hi Arnaldo,
On Thu, Jul 19, 2018 at 03:31:14PM -0300, Arnaldo Carvalho de Melo wrote: > 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.
Something like this?
static struct comm_str *comm_str__get(struct comm_str *cs) { if (cs && refcount_inc_not_zero(&cs->refcnt)) return cs; return NULL; }
Other than that I don't have better idea, so
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks, Namhyung
| |