Re: [PATCH 1/4] dynamic_debug: consolidate repetitive struct _ddebug descriptor definitions

[Jason Baron <jbaron@redhat.com>]

On Fri, Sep 09, 2011 at 01:23:43PM -0600, Jim Cromie wrote:
> On Fri, Sep 9, 2011 at 4:31 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> > On Fri, Sep 9, 2011 at 6:02 AM, Joe Perches <joe@perches.com> wrote:
> >> On Thu, 2011-09-08 at 20:42 -0700, Andrew Morton wrote:
> >> > On Thu, 08 Sep 2011 19:13:16 -0700 Joe Perches <joe@perches.com> wrote:
> >> > > On Thu, 2011-09-08 at 16:52 -0700, Andrew Morton wrote:
> >> > > > On Tue, 30 Aug 2011 14:28:41 -0400
> >> > > > Jason Baron <jbaron@redhat.com> wrote:
> >> > > > > Replace the repetitive struct _ddebug descriptor definitions with
> >> > > > > a new DECLARE_DYNAMIC_DEBUG_META_DATA(name, fmt) macro.
> >> > > > > +#define DECLARE_DYNAMIC_DEBUG_METADATA(name, fmt)              \
> >> > > > > +       static struct _ddebug __used __aligned(8)               \
> >> > > > > +       __attribute__((section("__verbose"))) name = {          \
> >> > > > > +               .modname = KBUILD_MODNAME,                      \
> >> > > > > +               .function = __func__,                           \
> >> > > > > +               .filename = __FILE__,                           \
> >> > > > > +               .format = (fmt),                                \
> >> > > > > +               .lineno = __LINE__,                             \
> >> > > > > +               .flags =  _DPRINTK_FLAGS_DEFAULT,               \
> >> > > > > +               .enabled = false,                               \
> >> > > > > +       }
> >> > > > <anal>That macro implements a definition, not a declaration</anal>
> >> > > Andrew, that's not quite true
> >> > It's precisely true.
> >> Not according to the c99 standard section 6.7
> >
> > Have you read that paragraph ? This is what I found in paragraph 6.7,
> > which confirms Andrews interpretation:
> >
> > <quote>
> > A declaration specifies the interpretation and attributes of a set of
> > identifiers. A definition of an identifier is a declaration for that
> > identifier that:
> > - for an object, causes storage to be reserved for that object;
> > - for a function, includes the function body;
> > - for an enumeration constant or typedef name, is the (only)
> > declaration of the identifier.
> > </quote>
> >
> > Bart.
> >
> 
> 
> I hesitate to churn this more (I have patchset to go on top of all this) but
> 
> Id like to see an INIT_DYNAMIC_DEBUG_METADATA,
> along with ability to expose the descriptor.
> 
> This would support pr_dbg_cont(), by letting it see/reuse
> the same descriptor that controls the pr_debug that started
> the multi-call message.  While this defeats the "atomicity"
> of buffering the entire message before calling printk,
> it does so only for the actual uses of KERN_CONT.
> 
> It also allows for "lite" usage of dynamic-debug,
> including 1..few descriptor per file or module to control all debug printing.
> As outlined, this "lite" usage is determined by the coder,
> it would be cool if it were more configurable than that,
> but I dont see how that would work atm.
> 
> 

We can expose the descriptor, but I that can wait for a folow-up
patchset, especially, since we don't have any consumers at the moment.

> Now that the worms have escaped the can, one other thought:
> 	unsigned int lineno:24;
> allows for insanely large files.  The largest in the tree is 29k,
> 16 bits would cover all files likely to be accepted in the future.
> Even allowing for never-to-be-submitted machine-generated code,
> Id think 18 bits would suffice.  ~250k lines should be enough ;-)
> 
> Given that my patchset adds flags-filtering
> ( echo mt+p > $CONTROL )
> The availability of user-flags, which do nothing but mark callsites,
> has some value - user can mark arbitrary sets of callsites,
> then enable/disable them as a group or subgroup:
> 
> echo function foo +x > $CONTROL
> ...
> echo x+p > $CONTROL
> echo y-p > $CONTROL
> echo z+p > $CONTROL
> echo yz-p > $CONTROL
> 
> Since since it works with module/file/function filtering, 3-4 user
> flags should be plenty.

makes sense...this can also be done is follow-on patchset...

thanks,

-Jason