linux-kernel - Re: [PATCH 5/7 v6] trace, RAS: Add eMCA trace event interface

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: <20140529074345.GA23484@gchen.bj.intel.com>
Date:	Thu, 29 May 2014 03:43:45 -0400
From:	"Chen, Gong" <gong.chen@...ux.intel.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Borislav Petkov <bp@...en8.de>, tony.luck@...el.com,
	m.chehab@...sung.com, linux-acpi@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/7 v6] trace, RAS: Add eMCA trace event interface

On Wed, May 28, 2014 at 12:56:25PM -0400, Steven Rostedt wrote:
> My concern is passing in a large string and wasting a lot of the ring
> buffer space. The max you can hold per event is just under a page size
> (4k). And all these strings add up. If it happens to be 512bytes, then
> you end up with one event per page.
I just don't understand why you say wasting memory. I just pass
a char * not a string array. And most of time these strings are partial full,
about 1/5 ~ 1/4 spaces are used.

> 
> Instead of making that a huge string, what about a dynamic array of
> special structures?
> 
> 
> struct __attribute__((__packed__)) cper_sec_mem_rec {
> 	short type;
> 	int data;
> };
> 
> 
> static struct cper_sec_mem_rec mem_location[CPER_REC_LEN];
> 
> then have the:
> 
>  	if (mem->validation_bits & CPER_MEM_VALID_NODE) {
> 		msg[n].type = CPER_MEM_VALID_NODE_TYPE;
> 		msg[n++].data = mem->node;
> 	}
> 	if (mem->validation_bits & CPER_MEM_VALID_CARD) {
> 		msg[n].type = CPER_MEM_VALID_CARD_TYPE;
> 		msg[n++].data = mem->card;
> 	}
> 	if (mem->validation_bits & CPER_MEM_VALID_MODULE) {
> [ and so on ]
> 

This function is not only for perf but for dmesg. So key is how
to handle two strings: dimm_location and mem_location.

I read some __print_symbolic implementations like btrfs trace,

#define show_ref_type(type)                                     \
        __print_symbolic(type,                                  \
        { BTRFS_TREE_BLOCK_REF_KEY,     "TREE_BLOCK_REF" },     \
        { BTRFS_EXTENT_DATA_REF_KEY,    "EXTENT_DATA_REF" },    \
        { BTRFS_EXTENT_REF_V0_KEY,      "EXTENT_REF_V0" },      \
        { BTRFS_SHARED_BLOCK_REF_KEY,   "SHARED_BLOCK_REF" },   \
        { BTRFS_SHARED_DATA_REF_KEY,    "SHARED_DATA_REF" })

So for this case, maybe we need a macro like:

#define show_dimm_location(type)                                \
        __print_symbolic(type,                                  \
        { CPER_MEM_VALID_NODE,          "node" },               \
        { CPER_MEM_VALID_CARD,          "card" },               \
        { CPER_MEM_VALID_MODULE,        "module" },             \
        ...

IMO, it is just another implementation method, maybe more graceful,
but I don't know how it can save space. Again, original functions
work both for trace and dmesg. If we add such interface, it looks
a little bit repeated.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)