lkml.org 
[lkml]   [2018]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-07-20 03:21    [W:0.082 / U:0.656 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site