Re: [PATCH] 2.5.44: lkcd (9/9): dump driver and build files

[Christoph Hellwig <hch@sgi.com>]

On Mon, Oct 21, 2002 at 03:06:59PM -0700, Piet Delaney wrote:
> > Using volatile is almost always a bug.  USe atomic variables
> > or bitops instead.
> 
> Yea, volatile is just being used to implement a simple atomic variable. 

It just isn't guaranteed to be atomic.. Use atomic_t (for actual
values) or unsigned long + set_bit/test_bit/ænd friends for bitmasks.

> > > +static dump_compress_t dump_none_compression = {
> > > +	compress_type:	DUMP_COMPRESS_NONE,
> > > +	compress_func:	dump_compress_none,
> > > +};
> > 
> > C99 initializers.
> 
> What's C99?

.name = value, i.e. in this case:

static dump_compress_t dump_none_compression = {
	.compress_type		= DUMP_COMPRESS_NONE,
	.compress_func		= dump_compress_none,
};

> > > +#if DUMP_DEBUG
> > > +void dump_bp(void) {}
> > > +#endif
> > 
> > Doesn't seem to be implemented at all.
> 
> .
> dump_bp() is just a debugging hook that's
> enabled while debugging the lkcd dump driver.
> DUMP_DEBUG can be defined in dump.h and allows
> the kernel and user space code to be instrumented
> for easier debugging. I found it useful while debugging.

Well, I can't find dump_bp beeing implemented at all.

> Perhaps the usefulness of the various aproaches during early startup
> should be considered. In RedHat 8.0 there does seem to be existant
> hooks for user space core dumps in /etc/sysctl.conf:
> ----------------------------------------------------------------------
>  Controls whether core dumps will append the PID to the core filename.
> # Useful for debugging multi-threaded applications.
> kernel.core_uses_pid =
> ----------------------------------------------------------------------
> When is /etc/sysctl.conf read by the kernel?

It isn't.  Ths userspace tool sysctl reads it if asked to.  It's done
in /etc/init.d/network by redhat.  But you can uses systcl with a config
file or directly specified parameters at any time you want - or just
write to the /proc/sys/* files (which is what sysctl does internally).

> It's a callback used by the dump driver. These callbacks are
> used by disk drivers, like the SGI SCSI driver, to tune the
> dump driver for it's needs. The callback facility was added
> to the dump driver and this tuning for the device driver seemed
> like a natural user of the facility. 

IMHO we should not have that code in the mainline kernel unless
uses.  And personally I can't see any reason why xscsi should be
ported to 2.6, although of course I can't don't know what plans
that team has nor do I know about any managment decisions.

> > Yuck.  Please just declare page_is_ram for every port.
> > 
> > > +#if !defined(CONFIG_DISCONTIGMEM) && !defined(CONFIG_IA64)
> > > +	p = (struct page *) &(mem_map[page_index]);
> > 
> > Yuck.  Please don't use mem_map in common code.
> 
> Yea the NUMA Ia64 code can't use it; I dind't see
> and easy way of replacing it. What do you suggest?

There is a node_mem_map in each pgdat_t - that's what the common
code uses nowdays.

> > Why don't you do this 
> 
> Perhaps it could. Perhaps kern_addr_valid() doesn't work for ia64.
> I'd have to check to be sure.

It should work on any architecture.  Once you're in mainline thay
just have to fix it if it doesn't :)

> > I don't think ioctl is the right interface.  Most of those want to be sysctls.
> 
> When can sysctls be set up? Earlier that ioctls() can be used? Perhaps
> something very early in startup should be used; like kernel boot
> options. 

sysctl by number is a syscall and can be used at any time.  sysctl
by name needs procfs mounted.  And of course you driver must either
in the kernel or the module loaded so that the entries are actually
registered.

> > 
> > Just move it to arch code for everyone.
> 
> What's wrong with having code which is common for most
> arch's in the dump driver. Seems to me it keeps things
> easier to understand. With ia64 being the only arch
> with special needs, it seems cleaner to just have common
> code handle the common situation.

ifdefs in common code make reading it a lot harder, and per-arch
ifdefs are a maintaince horror.  Maybe introduce a __ARCH_DUMP_MBANK
in asm-ia64/dump.h instead so any new arch can plug in easily?

> > > +	/* try to create our dump device */
> > > +	if (unregister_chrdev(DUMP_MAJOR, "dump"))
> > > +		DUMP_PRINT("cannot unregister dump character device!\n");
> > 
> > the printk is rather pointless..
> 
> Why is that?

Because the module will be unloaded anyway - i.e. the admin can do
anything at all if it failed.  For the given arguments it can't
return an error anyway - I think we should change it to void return
for 2.6..

> > > +kdev_t dump_dev;   	           /* the actual kdev_t device number      */
> > 
> > didn't we already have this one in dump_base.c?
> I thought this is dump_base.c

Exactly the same line is in dump_base.c and dump_blockdev.c.

> > Shouldn't much of this be static? again volatile is seldomly a good idea.
> 
> I suppose  with 'bio_complete' being an int changing it's value is
> atomic as long as it's treated a a volatile.Using a spinlock might
> be cleaner.

an atomic_t or set_bit/test_bit/etc is used to be atomic in the kernel
if you don't want a lock.  And in this case a lock looks like overkill.