[sheepdog] [PATCH 0/8] improve reference counting of data structures

[Hitoshi Mitake]

At Tue, 09 Jul 2013 12:24:03 +0900,
MORITA Kazutaka wrote:
> 
> At Tue, 9 Jul 2013 10:48:55 +0800,
> Liu Yuan wrote:
> > 
> > On Tue, Jul 09, 2013 at 11:36:34AM +0900, Hitoshi Mitake wrote:
> > > Current sheep uses raw int and uatomic_{inc,dec} for reference
> > > counting of data structures. This is not a good practice because:
> > > 1. We can simply increment or decrement with ++ or -- operators of C
> > >    because these counters has a type of int. It causes typical
> > >    multithreading programming errors.
> > > 2. In sheep, only main thread is allowed to operate reference counts
> > >    of some data structures (e.g. client_info and request). But in
> > >    current sheep, reference counters for these structs also employ
> > >    uatomic_{inc,dec}. This is not necessary and confusing.
> > > 
> > > This patch implements two new data structures for reference counting:
> > > refcnt_t and main_refcnt_t. refcnt_t can be used for structs which can
> > > be get or put by multiple threads. main_refcnt_t can be used for
> > > structs which can be used by the main thread. These types leverage
> > > compiler support for detecting bad operations and manipulated safely
> > > than previous reference counters whose types are raw integer.
> > > 
> > 
> > There xxx_t wrapper makes things more complex and look unnecessary to me. I think 
> > uatomic_{inc, dec} is good enough to understand. And if you think some refcount
> > used in main thread only, we can simply unfold them into plain ++/--, rather
> > than introducing more construct.
> 
> I think the aim of his patches is not to improve code readability, but
> to add a type check for reference counting and to detect wrong use of
> refcnts.
> 
> IMO, this series looks overkill.  Using uatomic_inc and uatomic_dec is
> always safe and cannot cause a performance problem.  If you want to
> use ++/-- for reference counting for some reasons, adding
> is_main_thread() or using main_thread_get() is enough to make sure we
> are in the main thread.

Generally speaking, uatomic_{inc,dec} incurs performance overhead
because they use atomic operations. The overhead produced by signle
calling of uatomic_{inc,dec} is not so large, but in some extreme
cases, it cannot be ignored.
# I'm not sure sheep can face this sort of performance problem.

Adding is_main_thread() or main_thread_get() by hand has a possibility
of mistakes. The motivation of this series is same to the one of
implementing uatomic_bool. The typedef of uatomic_bool prevents direct
assignment of true. These typedefs are effective for human
errors. And checking of minus value of reference counters can also be
done by the wrappers. And the last patch of this series shows the
effect of this way.

Thanks,
Hitoshi