Re: Listing 9.2: actual free(3) will probably cause problem

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Sat, Oct 28, 2017 at 05:29:34PM +0800, Yubin Ruan wrote:
> Hi Paul,
> 
> I would suggest the following patch for defer/route_refcnt.c (as in Listing
> 9.2). For the change in re_free, if we actually call free(3) on `rep', we will
> lose that piece of memory so that the READ_ONCE() at line 36 might see garbage
> value and will probably will not call abort(3), i.e., that is implementation
> specific behavior. And I don't see why will we have `old <=0`.
> 
> I think I understand what that code mean but you might mean some other?
> Hopefully I will not be too picky.

Yes, this code is buggy and fixing it is on my list.  I would be happy to
accept a patch doing a real fix, but it is not simple.

One place to start is Anthony Williams's atomic shared pointer code.
The idea would be to translate that from C++ to C.

But please be aware that this is a non-trivial problem.  If you try to
hack together a solution, it -will- have subtle bugs.  So validating
your solution (even if you start from Anthony's approach) will be
non-trivial.  I suggest using cbmc or something similar in addition to
perfbook's stress tests.

So, are you up for it?  ;-)

							Thanx, Paul

> Thanks,
> Yubin
> 
> ----------------------------------------
> diff --git a/CodeSamples/defer/route_refcnt.c b/CodeSamples/defer/route_refcnt.c
> index 8a48faf..0d24e9d 100644
> --- a/CodeSamples/defer/route_refcnt.c
> +++ b/CodeSamples/defer/route_refcnt.c
> @@ -36,7 +36,8 @@ DEFINE_SPINLOCK(routelock);
>  static void re_free(struct route_entry *rep)
>  {
>  	WRITE_ONCE(rep->re_freed, 1);
> -	free(rep);
> +    /* Will not actually free it. Just use the `re_freed' as a flag */
> +	/*free(rep);*/
>  }
> 
>  /*
> @@ -50,7 +51,6 @@ unsigned long route_lookup(unsigned long addr)
>  	struct route_entry **repp;
>  	unsigned long ret;
> 
> -retry:
>  	repp = &route_list.re_next;
>  	rep = NULL;
>  	do {
> @@ -65,8 +65,6 @@ retry:
>  			if (READ_ONCE(rep->re_freed))
>  				abort();
>  			old = atomic_read(&rep->re_refcnt);
> -			if (old <= 0)
> -				goto retry;
>  			new = old + 1;
>  		} while (atomic_cmpxchg(&rep->re_refcnt, old, new) != old);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe perfbook" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>