Re: [patch 2/5] try2: x86_64: CPU hotplug support.

[Ashok Raj]

On Mon, Jun 06, 2005 at 03:11:56PM -0700, Andrew Morton wrote:
> Ashok Raj <ashok.raj@xxxxxxxxx> wrote:
> >
> > Experimental CPU hotplug patch for x86_64
> 
> What does "experimental" mean?

Well, stictly since these sections are still under CONFIG_EXPERIMENTAL
in arch/x86_64/Kconfig. Evolving... ? 

> 
> > +
> > +	if (!keventd_up() || current_is_keventd())
> > +		work.func(work.data);
> > +	else {
> > +		schedule_work(&work);
> > +		wait_for_completion(&c_idle.done);
> > +	}
> 
> This shouldn't be diddling with workqueue internals.  Why is this code
> here?  If the workqueue API is inadequate then we should prefer to extend
> it rather than working around any shortcoming.

This has been around for ages.. even in ia64 code. For forking idle threads
we want to do them in clean state so we dont acquire state from threads
from where the cpu_up is being invoked. Hence we want them to start from
keventd() threads. But when system boot is happening, there is no keventd()
yet, hence we need to create them right away.

the other problem we ran into was ACPI code that handles physical cpu hotplug
also queues to keventd(), this becomes permanently blocking when called from
code already running in kevend(). 

> > +		Dprintk ("do_boot_cpu %d Already started\n", cpu);
> 
> Please try to adopt a consistent coding style.

I was actually trying to be consistent :-), rest of the debug code 
was under Dprintk() hence didnt want to use a new style. Not sure
what you need here exactly. Do you want to convert the rest of the code to 
not use Dprintk()? or just leave this with a printk? I dont have a 
particular preference here... i would rather leave it with Dprintk() as the
rest of the debug code.
> 
> Using printk("%s", __FUNCTION__); is preferred, as it will still work if
> someone later refactors this code into a new function.  (It can increase
> code size.  Or decrease it if the string gets shared.  But that's moot if
> the code is inside a normally-disabled macro like Dprintk.  Whatever that
> is.)
> 
> > +static void
> > +remove_siblinginfo(int cpu)
> 
> Unneeded newline here.

I can remove.. when iam inside the file, iam used to search for fn;s from 
start of line.. no biggie.. can revert.

Cheers,
ashok
-
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/