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

[Hitoshi Mitake]

At Tue, 09 Jul 2013 14:35:16 +0900,
MORITA Kazutaka wrote:
> 
> At Tue, 09 Jul 2013 14:20:10 +0900,
> Hitoshi Mitake wrote:
> > 
> > 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.
> 
> Please give us the number if the aim of this series is performance
> improvement.

The aim of this patch is not related to performance. Sorry, the above
comments are off the subject.

> 
> > 
> > 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.
> 
> I'm not against introducing wrapper functions to increment and
> decrement counters, but introducing two types of counters still looks
> complicated and overkill to me.  Using uatomic operations always looks
> enough.

Wrapping variables manipulated atomically is a common practice of
parallel programming. e.g. urcu implements its own refcnt_t for
internal use (we can't use it for sheepdog because the function of
urcu for decrement requires callback for freeing objects). And linux
kernel implements atoimc_t for atomic variables. This sort of
techniques free us from begin careful about not incrementing and
decrementing atomic variables with ++ and --.

I believe that we shouldn't use uatomic_{inc,dec} directly. In the
future, I want to delete every call of uatomic_{inc,dec} from .c files
of sheepdog.

Thanks,
Hitoshi