Re: [5/7, v9] NUMA Hotplug Emulator: Support cpu probe/release in x86_64

[Shaohui Zheng <shaohui.zheng@linux.intel.com>]

On Wed, Dec 22, 2010 at 07:21:18PM -0800, Andrew Morton wrote:
> > > 
> > > checkpatch?
> > 
> > it is a warning, so I ignore it.
> 
> Don't ignore warnings!  At least, not until you've understood the
> reason for them and have a *reason* to ignore them.
> 
> simple_strtoul() will silently accept input of the form "42foo",
> treating it as "42".  That's a userspace bug and the kernel should
> report it.  This means that the code should be changed to handle error
> returns from strict_strtoul().  And those error paths should be tested.
> 

> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (selected >= num_possible_cpus()) {
> > > > +		printk(KERN_ERR "No free cpu, give up cpu probing.\n");
> > > > +		return -EPERM;
> > > > +	}
> > > > +
> > > > +	/* register cpu */
> > > > +	arch_register_cpu_node(selected, nid);
> > > > +	acpi_map_lsapic_emu(selected, nid);
> > > > +
> > > > +	return count;
> > > > +}
> > > > +EXPORT_SYMBOL(arch_cpu_probe);
> > > 
> > > arch_cpu_probe() is global and exported to modules, but is undocumented.
> > > 
> > > If it had been documented, I might have been able to work out why arg
> > > `count' is checked, but never used.
> > > 
> > 
> > Sorry, Andrew, I did not catch it. Do you mean to add the document before
> >  the definition of the function arch_cpu_probe?
> 
> Sure, add a comment documenting the function.

I understand, I will add comments for both arch_cpu_probe/arch_cpu_release.

> 
> Why *does* it check `count' and then not use it?
> 

it is a tricky thing. When I debug it under a Virtual Machine, If I do a cpu
probe via sysfs cpu/probe interface, The function arch_cpu_probe will be called
__three__ times, but only one call is valid, so I add a check on `count` to
ignore the invalid calls.

> > 
> > > > +	/* cpu 0 is not hotplugable */
> > > > +	if (cpu == 0) {
> > > > +		printk(KERN_ERR "can not release cpu 0.\n");
> > > 
> > > It's generally better to make kernel messages self-identifying. 
> > > Especially error messages.  If someone comes along and sees "can not
> > > release cpu 0" in their logs, they don't have a clue what caused it
> > > unless they download the kernel sources and go grepping.
> > > 
> > 
> > How about "arch_cpu_release: can not release cpu 0.\n"?
> 
> Better, although "arch_cpu_release" isn't very meaningful to an
> administrator.  "NUMA hotplug remove" or something like that would be
> more useful.

> 
> All these messages should be looked at from the point of view of the
> people who they are to serve.  Although in this special case, that's
> most likely to be a kernel developer so I guess such clarity isn't
> needed.
> 

It is a good lesson for me, when I meet the similar problem next time, I should
consider more from the point of the user.

-- 
Thanks & Regards,
Shaohui