linux-kernel - Re: [patch] x86: reduce srat verbosity in the kernel log

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: <alpine.DEB.2.00.0911130158390.13911@chino.kir.corp.google.com>
Date:	Fri, 13 Nov 2009 02:02:17 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Ingo Molnar <mingo@...e.hu>
cc:	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mike Travis <travis@....com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Roland Dreier <rdreier@...co.com>,
	Randy Dunlap <rdunlap@...otime.net>, Tejun Heo <tj@...nel.org>,
	Andi Kleen <andi@...stfloor.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Yinghai Lu <yhlu.kernel@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>,
	Jack Steiner <steiner@....com>,
	Frederic Weisbecker <fweisbec@...il.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch] x86: reduce srat verbosity in the kernel log

On Fri, 13 Nov 2009, Ingo Molnar wrote:

> > It's possible to reduce the number of SRAT messages emitted to the kernel
> > log by printing each valid pxm once and then creating bitmaps to represent
> > the apic ids that map to the same node.
> > 
> > This reduces lines such as
> > 
> > 	SRAT: PXM 0 -> APIC 0 -> Node 0
> > 	SRAT: PXM 0 -> APIC 1 -> Node 0
> > 	SRAT: PXM 1 -> APIC 2 -> Node 1
> > 	SRAT: PXM 1 -> APIC 3 -> Node 1
> > 
> > to
> > 
> > 	SRAT: PXM 0 -> APIC {0-1} -> Node 0
> > 	SRAT: PXM 1 -> APIC {2-3} -> Node 1
> > 
> > The buffer used to store the apic id list is 128 characters in length.
> > If that is too small to represent all the apic id ranges that are bound
> > to a single pxm, a trailing "..." is added.  APICID_LIST_LEN should be
> > manually increased for such configurations.
> > 
> > Acked-by: Mike Travis <travis@....com>
> > Signed-off-by: David Rientjes <rientjes@...gle.com>
> > ---
> >  arch/x86/mm/srat_64.c |   41 +++++++++++++++++++++++++++++++++++++----
> >  drivers/acpi/numa.c   |    5 +++++
> >  include/linux/acpi.h  |    3 ++-
> >  3 files changed, 44 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c
> > --- a/arch/x86/mm/srat_64.c
> > +++ b/arch/x86/mm/srat_64.c
> > @@ -36,6 +36,9 @@ static int num_node_memblks __initdata;
> >  static struct bootnode node_memblk_range[NR_NODE_MEMBLKS] __initdata;
> >  static int memblk_nodeid[NR_NODE_MEMBLKS] __initdata;
> >  
> > +static DECLARE_BITMAP(apicid_map, MAX_LOCAL_APIC) __initdata;
> > +#define APICID_LIST_LEN	(128)
> > +
> >  static __init int setup_node(int pxm)
> >  {
> >  	return acpi_map_pxm_to_node(pxm);
> > @@ -136,8 +139,6 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
> >  	apicid_to_node[apic_id] = node;
> >  	node_set(node, cpu_nodes_parsed);
> >  	acpi_numa = 1;
> > -	printk(KERN_INFO "SRAT: PXM %u -> APIC %u -> Node %u\n",
> > -	       pxm, apic_id, node);
> >  }
> >  
> >  /* Callback for Proximity Domain -> LAPIC mapping */
> > @@ -170,8 +171,40 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
> >  	apicid_to_node[apic_id] = node;
> >  	node_set(node, cpu_nodes_parsed);
> >  	acpi_numa = 1;
> > -	printk(KERN_INFO "SRAT: PXM %u -> APIC %u -> Node %u\n",
> > -	       pxm, apic_id, node);
> > +}
> > +
> > +void __init acpi_numa_print_srat_mapping(void)
> > +{
> > +	char apicid_list[APICID_LIST_LEN];
> > +	int i, j;
> > +
> > +	for (i = 0; i < MAX_PXM_DOMAINS; i++) {
> > +		int len;
> > +		int nid;
> > +
> > +		nid = pxm_to_node(i);
> > +		if (nid == NUMA_NO_NODE)
> > +			continue;
> > +
> > +		bitmap_zero(apicid_map, MAX_LOCAL_APIC);
> > +		for (j = 0; j < MAX_LOCAL_APIC; j++)
> > +			if (apicid_to_node[j] == nid)
> > +				set_bit(j, apicid_map);
> > +
> > +		if (bitmap_empty(apicid_map, MAX_LOCAL_APIC))
> > +			continue;
> > +
> > +		/*
> > +		 * If the bitmap cannot be listed in a buffer of length
> > +		 * APICID_LIST_LEN, then it is suffixed with "...".
> > +		 */
> > +		len = bitmap_scnlistprintf(apicid_list, APICID_LIST_LEN,
> > +					   apicid_map, MAX_LOCAL_APIC);
> > +		pr_info("SRAT: PXM %u -> APIC {%s%s} -> Node %u\n",
> > +			i, apicid_list,
> > +			(len == APICID_LIST_LEN - 1) ? "..." : "",
> > +			nid);
> > +	}
> >  }
> 
> No. As i suggested many times before, just get rid of the printouts or 
> make them boot-debug-flag dependent.
> 

Hmm, so even if these were dependent on a kernel parameter, do you think 
this patch would still be needed exactly as it is written?  In other 
words, do you believe that Mark's system should really emit 1272 lines for 
this data instead of the 40 with this patch?

I'm all for making this dependent on a kernel parameter, but it's a 
seperate change than what this patch is addressing.  We need the apic id 
mappings because they aren't exposed to userspace in any other way, 
they need to exported somehow and this is a clear improvement to reduce 
verbosity in the kernel log.

So I really don't know what you're insisting here, you want this 
compression to be accompanied by another patch which makes the call to 
acpi_numa_print_srat_mapping() depend on a kernel parameter?  Or are you 
saying we shouldn't compress it at all and Mike should just deal with 1272 
lines instead of 40?  Or are you saying we should export this static 
information via sysfs?
--
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/