Re: proposal for fine-grain dout control

[caifeng.zhu@uniswdc.com]

On Fri, Nov 18, 2016 at 03:04:38PM +0100, Ilya Dryomov wrote:
> On Thu, Nov 17, 2016 at 5:32 AM,  <caifeng.zhu@uniswdc.com> wrote:
> > Currently dout control is based on module and trace level.
> > In practice, we find that it is much easier to control dout
> > by specifying module, file, function and line, as done by
> > kernel rbd/cephfs client module. The paragraphs below try to
> > propose a way to implement similar control at the ceph server
> > side.
> >
> > As the kernel clients, we can define control points (called
> > doutpoints later) for dout in the macro dout_impl and collect
> > them into a seperate ELF section, like below:
> >
> >  #define dout_impl(cct, sub, v)                                         \
> >    do {                                                                 \
> > -  if (cct->_conf->subsys.should_gather(sub, v)) {                      \
> > +  static doutpoint_t *_dop;                                            \
> > +  DOUTPOINT(TOSTR(sub), __FILE__, __func__, __LINE__);                 \
> > +                                                                       \
> > +  if (cct->_conf->subsys.should_gather(sub, v) ||                      \
> > +      doutpoint_active(_dop)) {                                        \
> >
> > In the above code, DOUTPOINT will define a doutpoint and ask linker
> > to collect it into an ELF section, such as "set_doutpoint". Instead of
> > adding __attribute__((section("set_doutpoint"))) to variables, we resort
> > to inline ASM like below:
> >
> > +typedef struct doutpoint {
> > +       const char *    dop_module;     // module
> > +       const char *    dop_file;       // file name
> > +       const char *    dop_func;       // function name
> > +       int             dop_line;       // line no
> > +       volatile int    dop_state;      // state 0:off, 1:on
> > +} doutpoint_t;
> >
> > +#define        TOSTR(a)        #a
> > +#define DOUTPOINT(module, file, func, line)                             \
> > +do {                                                                    \
> > +   __asm__(".pushsection set_doutpoint, \"aw\", @progbits" "\n"         \
> > +           ".align 32"   "\n"                                           \
> > +           "416:"        "\n"                                           \
> > +           ".quad %c1"   "\n"                                           \
> > +           ".quad %c2"   "\n"                                           \
> > +           ".quad %c3"   "\n"                                           \
> > +           ".long %c4"   "\n"                                           \
> > +           ".long 0"     "\n"                                           \
> > +           ".popsection" "\n"                                           \
> > +           "leaq 416b(%%rip), %0" "\n"                                  \
> > +           : "=r"(_dop)                                                 \
> > +           : "i"(module), "i"(file), "i"(func), "i"(line));             \
> > +} while(0)
> >
> > The reason for inline ASM is that: in C++, there are inline and template
> > functions. static variables defined in these functions will be automatically
> > added attributes by the compiler. thus it impossible to collect all doutpoints
> > into one section.
> >
> > There is another problem to consider: collect doutpoints of shared libs into
> > the main executable. We solve the problem by defining a constructor function
> > and a weak function. The contructor function will automatically register doupoints
> > of shared libs into the main executable. The weak function is to be overriden by
> > the main executable, providing where to register doutponits.
> >
> > //
> > // in common/dout.h
> > //
> > +typedef struct doutsection {
> > +       doutpoint_t *   start;
> > +       doutpoint_t *   stop;
> > +} doutsection_t;
> >
> > //
> > // in common/dout.cc
> > //
> > +extern doutpoint_t __start_set_doutpoint[]
> > +       __attribute__((weak, visibility("hidden")));
> > +extern doutpoint_t __stop_set_doutpoint[]
> > +       __attribute__((weak, visibility("hidden")));
> > +
> > +extern void doutsection_global(doutsection_t **s, int *n)
> > +       __attribute__((weak));
> > +
> > +
> > +static void
> > +doutsection_array(doutsection_t **s, int *n)
> > +{
> > +       static doutsection_t _sections[10];
> > +
> > +       /*
> > +        * If doutsection_global is defined, provide
> > +        * from the global one; otherwise, fall back
> > +        * to local ones.
> > +        */
> > +       if (doutsection_global)
> > +               doutsection_global(s, n);
> > +       else {
> > +               *s = _sections;
> > +               *n = 10;
> > +       }
> > +}
> > +
> > +/*
> > + * The 'contructor' attribute means the function will be
> > + * called after dynamic linking for the library or before
> > + * entering into main for the executable.
> > + */
> > +void __attribute__((constructor))
> > +doutpoint_register(void)
> > +{
> > +       int i, n;
> > +       doutsection_t *s;
> > +
> > +       doutsection_array(&s, &n);
> > +       for (i = 0; i < n; i++, s++) {
> > +               if (!s->start) {
> > +                       s->start = __start_set_doutpoint;
> > +                       s->stop = __stop_set_doutpoint;
> > +                       return;
> > +               }
> > +       }
> > +}
> >
> > //
> > // in global/global_init.cc
> > //
> > void
> > doutsection_global(doutsection_t **s, int *n)
> > {
> >         static doutsection_t _sections[10];
> >
> >         *s = _sections;
> >         *n = 10;
> > }
> >
> > There are some shortcomings of the proposal: mainly archtecture
> > dependency and compiler dependency. DOUTPOINT exploits one x86
> > instruction; weak symbol support may be different on different
> > compilers.
> >
> > Glad and eager to hear your opinions!
> > If it is useful, I'll post the whole patch.
> 
> As someone who has experience with kernel dynamic_debug mechanism,
> I wish we had a subsystem/level mechanism in the kernel client instead.
> While sometimes handy, enabling only a few selected douts doesn't come
> useful that often - more frequently the dout you really need isn't
> there and you end up fiddling with "perf probe" and others, longing for
> a good scriptable dynamic tracer.
> 
> It looks like you are proposing this as an addition to the existing
> module/level scheme and not as a replacement though, so it's just an
> observation...

Yes, the proposal is an improvement, rather than a replacement. 

When analyzing a problem, we usually started with gross-grained info. After
some analysis and study, we can pinpoint the problem. At that time,we need 
fine-grained info. For the current dout, when pinpointing a problem, usually 
I'm flooded with too much info. By fine-grained dout control, it is much
easier to get what I really want.

> 
> Thanks,
> 
>                 Ilya
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html