[sheepdog] [PATCH 1/4] sheep: add preliminary stat function

[MORITA Kazutaka]

At Mon, 26 Aug 2013 13:48:51 +0800,
Liu Yuan wrote:
> 
> On Mon, Aug 26, 2013 at 02:46:24PM +0900, MORITA Kazutaka wrote:
> > At Mon, 26 Aug 2013 13:21:18 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Mon, Aug 26, 2013 at 10:39:26AM +0900, Hitoshi Mitake wrote:
> > > > At Sun, 25 Aug 2013 20:27:27 +0800,
> > > > Liu Yuan wrote:
> > > > > 
> > > > > Add a new struct sd_stat to monitor the activities of the sheep daemon, which
> > > > > are read by the dog. Currently it is very crude and monitors only request.
> > > > > 
> > > > > Signed-off-by: Liu Yuan <namei.unix at gmail.com>
> > > > > ---
> > > > >  include/internal_proto.h |   13 +++++++++++++
> > > > >  sheep/request.c          |   33 +++++++++++++++++++++++++++++++++
> > > > >  sheep/sheep_priv.h       |    1 +
> > > > >  3 files changed, 47 insertions(+)
> > > > > 
> > > > > diff --git a/include/internal_proto.h b/include/internal_proto.h
> > > > > index 2c24a52..adb9646 100644
> > > > > --- a/include/internal_proto.h
> > > > > +++ b/include/internal_proto.h
> > > > > @@ -215,4 +215,17 @@ struct object_cache_info {
> > > > >  	uint8_t directio;
> > > > >  };
> > > > >  
> > > > > +struct sd_stat {
> > > > > +	struct s_request{
> > > > > +		uint64_t gway_active_nr; /* nr of running request */
> > > > > +		uint64_t peer_active_nr;
> > > > > +		uint64_t gway_total_nr; /* Total nr of requests received */
> > > > > +		uint64_t peer_total_nr;
> > > > > +		uint64_t gway_total_rx; /* Data in */
> > > > > +		uint64_t gway_total_tx; /* Data out */
> > > > > +		uint64_t peer_total_rx;
> > > > > +		uint64_t peer_total_tx;
> > > > > +	} r;
> > > > > +};
> > > > > +
> > > > >  #endif /* __INTERNAL_PROTO_H__ */
> > > > > diff --git a/sheep/request.c b/sheep/request.c
> > > > > index a79f648..112258a 100644
> > > > > --- a/sheep/request.c
> > > > > +++ b/sheep/request.c
> > > > > @@ -315,6 +315,35 @@ static void queue_local_request(struct request *req)
> > > > >  	queue_work(sys->io_wqueue, &req->work);
> > > > >  }
> > > > >  
> > > > > +static inline void stat_request_begin(struct request *req)
> > > > > +{
> > > > > +	struct sd_req *hdr = &req->rq;
> > > > > +
> > > > > +	if (is_peer_op(req->op)) {
> > > > > +		sys->stat.r.peer_total_nr++;
> > > > > +		sys->stat.r.peer_active_nr++;
> > > > > +		if (hdr->flags & SD_FLAG_CMD_WRITE)
> > > > > +			sys->stat.r.peer_total_rx += hdr->data_length;
> > > > > +		else
> > > > > +			sys->stat.r.peer_total_tx += hdr->data_length;
> > > > > +	} else if(is_gateway_op(req->op)) {
> > > > > +		sys->stat.r.gway_total_nr++;
> > > > > +		sys->stat.r.gway_active_nr++;
> > > > > +		if (hdr->flags & SD_FLAG_CMD_WRITE)
> > > > > +			sys->stat.r.gway_total_rx += hdr->data_length;
> > > > > +		else
> > > > > +			sys->stat.r.gway_total_tx += hdr->data_length;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static inline void stat_request_end(struct request *req)
> > > > > +{
> > > > > +	if (is_peer_op(req->op))
> > > > > +		sys->stat.r.peer_active_nr--;
> > > > > +	else if(is_gateway_op(req->op))
> > > > > +		sys->stat.r.gway_active_nr--;
> > > > > +}
> > > > > +
> > > > 
> > > > The above two functions should be annotated with main_fn. BTW, why
> > > > should they be inline?
> > > 
> > > I'm wondering, do we need to mark every functions in main thread as main_fn? I
> > 
> > If you don't guard sys->stat.* with a lock, they must be called in the
> > main thread.  I think this is an important case and suggest adding
> > main_fn to them.
> > 
> > > think only some important ones, by the way, if marked as inline, we can't
> > > mark them as main_fn.
> > 
> > Why?  I added main_fn to them but didn't hit any problems.
> > 
> >   main_fn static inline void stat_request_end(struct request *req)
> > 
> >   main_fn static inline void stat_request_end(struct request *req)
> 
> No problems but if they are inlined after complied, how does trace checker check
> them? There are no symbols of them and or instrumented by mcount.

Ah, I see the point, but the thread checker is only for debugging,
right?  'inilne' is ignored when we disable an optimization with
--enable-debug, and I think it's make sense to add main_fn to inline
functions.  In addition, 'inline' is just a hint to GCC, and there is
no guarantee that the function will be inlined even with the release
build.

Thanks,

Kazutaka