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: <20101223022428.GB12333@shaohui>
Date:	Thu, 23 Dec 2010 10:24:28 +0800
From:	Shaohui Zheng <shaohui.zheng@...ux.intel.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
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 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

--
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/