Re: [PATCH perf/core 00/22] perf refcnt debugger API and fixes

[Arnaldo Carvalho de Melo]

Em Wed, Dec 09, 2015 at 10:41:38AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Dec 09, 2015 at 11:10:48AM +0900, Masami Hiramatsu escreveu:
> >   General refcnt miscodings
> >   =========================
> > 
> > BTW, while applying this change, I've found that there are refcnt
> > coding mismatches in those code and most of the bugs come from those
> > mismatches.
> > 
> > - The reference counter can be initialized by 0 or 1.
> >  - If 0 is chosen, caller have to get it and free it if failed to
> >    register appropriately.
> >  - If 1 is chosen, caller doesn't need to get it, but when exits the
> >    caller, it has to put it. (except for returning the object itself)
> > - The application should choose either one as its policy, to avoid
> >   confusion.
> > 
> > perf tools mixes it up (moreover, it initializes 2 in a case) and
> > caller usually forgets to put it (it is not surprising, because too
> > many "put" usually cause SEGV by accessing freed object.)
> 
> Well, we should fix the bugs and document why some initialization is
> deemed better than a single initialization style.
 
> For instance, if we know that we will keep multiple references straight
> away, then we could init it with some value different that preferred,
> that I aggee, is 1, i.e. the usual way for some constructor is:
> 
> 
> struct foo *foo__new()
> {
> 	struct foo *f = malloc(sizeof (*f));
> 
> 	if (f) {
> 		atomic_set(&f->refcnt, 1);
> 	}
> 
> 	return f;
> }
> 
> void *foo__delete(struct foo *f)
> {
> 	free(f);
> }
> 
> void foo__put(struct foo *f)
> {
> 	if (f && atomic_dec_and_test(f->refcnt))
> 		foo__delete(f);
> }
> 
> 
> Then, when using if, and before adding it to any other tree, list, i.e.
> a container, we do:
> 
> 	struct foo *f = foo__new();
> 
> 	/*
> 	 * assume f was allocated and then the function allocating it
> 	 * failed, when it bails out it should just do:
>  	 */
> out_error:
> 	foo__put(f);
> 
> And that will make it hit zero, which will call foo__delete(), case
> closed.
>  
> > As far as I can see, cgroup_sel, perf_mmap(this is initialized 0 or 2...),
> > thread, and comm_str are initialized by 0. Others are initialized by 1.
> > 
> > So, I'd like to suggest that we choose one policy and cleanup the code.
> > I recommend to use init by 1 policy, because anyway caller has to get
> 
> See above, if nothing else recommends using a different value, use 1.

So, I think I fixed the thread->refcnt case, please take a look at my
perf/core branch, more specifically this patch: