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

[Hitoshi Mitake]

At Tue, 09 Jul 2013 15:31:42 +0900,
MORITA Kazutaka wrote:
> 
> At Tue, 09 Jul 2013 15:08:26 +0900,
> Hitoshi Mitake wrote:
> > 
> > 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.
> 
> I'm not against both introducing wrapper functions and a type for
> reference count.  What I'm against is introducing two types for
> reference counts, refcnt_t and main_refcnt_t.

OK, I'll remove main_refcnt_t and simplify this patchset. I'll send v2
later.

Thanks,
Hitoshi