linux-kernel - Re: [5/7, v9] NUMA Hotplug Emulator: Support cpu probe/release in x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20101222192118.2d286ca9.akpm@linux-foundation.org>
Date:	Wed, 22 Dec 2010 19:21:18 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Shaohui Zheng <shaohui.zheng@...ux.intel.com>
Cc:	shaohui.zheng@...el.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, haicheng.li@...ux.intel.com,
	lethal@...ux-sh.org, ak@...ux.intel.com, rientjes@...gle.com,
	dave@...ux.vnet.ibm.com, gregkh@...e.de,
	Ingo Molnar <mingo@...e.hu>, Len Brown <len.brown@...el.com>,
	Yinghai Lu <Yinghai.Lu@....COM>, Tejun Heo <tj@...nel.org>,
	Haicheng Li <haicheng.li@...el.com>
Subject: Re: [5/7, v9] NUMA Hotplug Emulator: Support cpu probe/release in
 x86_64

On Thu, 23 Dec 2010 09:34:10 +0800 Shaohui Zheng <shaohui.zheng@...ux.intel.com> wrote:

> On Wed, Dec 22, 2010 at 04:27:27PM -0800, Andrew Morton wrote:
> > On Fri, 10 Dec 2010 15:31:24 +0800
> > > +
> > > +ssize_t arch_cpu_probe(const char *buf, size_t count)
> > > +{
> > > +	int nid = 0;
> > > +	int num = 0, selected = 0;
> > 
> > One definition per line make for more maintainable code.
> > 
> > Two of these initialisations are unnecessary.
> > 
> Agree, I will put them into 2 lines, and remove the initialisations.
> I always try to initialize them when we define it, it seems that it is a bad habit.
> 
> > > +	/* check parameters */
> > > +	if (!buf || count < 2)
> > > +		return -EPERM;
> > > +
> > > +	nid = simple_strtoul(buf, NULL, 0);
> > 
> > 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.

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

> 
> > > +	/* 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.


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