Re: [PATCH] CKRM: 5/10 CKRM: Task based management for CPU, memory and Disk I/O.

[Gerrit Huizenga]

On Mon, 29 Nov 2004 14:23:23 PST, Greg KH wrote:
> On Mon, Nov 29, 2004 at 10:49:09AM -0800, Gerrit Huizenga wrote:
> > +#define TC_DEBUG(fmt, args...) do { \
> > +/* printk("%s: " fmt, __FUNCTION__ , ## args); */ } while (0)
> 
> Again with the new debug macro :(
> 
> > +static struct ckrm_task_class taskclass_dflt_class = {
> > +};
> 
> Empty structure?  Why?
 
Initialized definition, not declaration.  Although with no initializer
which was a bit odd.  struct ckrm_task_class is defined in ckrm_tc.h.

> > +// Hubertus .. following functions should move to ckrm_rc.h
> 
> Why haven't they moved :)

Because we aren't done yet.  ;-)

> > +static inline void ckrm_task_lock(struct task_struct *tsk)
> > +{
> > +	spin_lock(&tsk->ckrm_tsklock);
> > +}
> 
> Just lock (or unlock) the lock, don't wrap a lock in a function.
 
Yep.  Done.

> > +DECLARE_MUTEX(async_serializer);	// serialize all async functions
> 
> Should this really be global?  The code says otherwise :)
 
Not any more.

> > +	printk("...... Initializing ClassType<%s> ........\n",
> > +	       CT_taskclass.name);
> 
> What a pretty log message.  Unfortunately it's wrong (me hears the
> growing mumblings of the kernel janitor mob...)
 
Yep - fixed.

> > +#if 0
> > +
> > +/******************************************************************************
> > + * Debugging Task Classes:  Utility functions
> > + ******************************************************************************/
> 
> Then remove the code, if it's not needed.
 
Okay.  I can easily carry a debug patch later.  Should have done that
sooner...

> > +EXPORT_SYMBOL(tcp_v4_lookup_listener);
> 
> Not EXPORT_SYMBOL_GPL()?
 
Currently makes it just like all the others.  I'll let the networking
folks chime in on how they want that exported when this patch gets
cross posted to netdev.

thanks,

gerrit
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/