linux-kernel - Re: [PATCH v3 08/23] mm/memblock: Add memblock memory allocation apis

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]

Date:	Sat, 14 Dec 2013 06:08:44 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Santosh Shilimkar <santosh.shilimkar@...com>
Cc:	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	linux-arm-kernel@...ts.infradead.org,
	Yinghai Lu <yinghai@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [PATCH v3 08/23] mm/memblock: Add memblock memory allocation apis

Hello, Santosh.

On Fri, Dec 13, 2013 at 07:52:42PM -0500, Santosh Shilimkar wrote:
> >> +static void * __init memblock_virt_alloc_internal(
> >> +				phys_addr_t size, phys_addr_t align,
> >> +				phys_addr_t min_addr, phys_addr_t max_addr,
> >> +				int nid)
> >> +{
> >> +	phys_addr_t alloc;
> >> +	void *ptr;
> >> +
> >> +	if (nid == MAX_NUMNODES)
> >> +		pr_warn("%s: usage of MAX_NUMNODES is depricated. Use NUMA_NO_NODE\n",
> >> +			__func__);
> > 
> > Why not use WARN_ONCE()?  Also, shouldn't nid be set to NUMA_NO_NODE
> > here?
> > 
> You want all the users using MAX_NUMNODES to know about it so that
> the wrong usage can be fixed. WARN_ONCE will hide that.

Well, it doesn't really help anyone to be printing multiple messages
without any info on who was the caller and if this thing is gonna be
in mainline triggering of the warning should be rare anyway.  It's
more of a tool to gather one-off cases in the wild.  WARN_ONCE()
usually is the better choice as otherwise the warnings can swamp the
machine and console output in certain cases.

> > ...
> >> +	if (nid != NUMA_NO_NODE) {
> > 
> > Otherwise, the above test is broken.
> > 
> So the idea was just to warn the users and allow them to fix
> the code. Well we are just allowing the existing users of using
> either MAX_NUMNODES or NUMA_NO_NODE continue to work. Thats what
> we discussed, right ?

Huh?  Yeah, sure.  You're testing @nid against MAX_NUMNODES at the
beginning of the function.  If it's MAX_NUMNODES, you print a warning
but nothing else, so the if() conditional above, which should succeed,
would fail.  Am I missing sth here?

Thanks.

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