Hi Joel,
I've reorganized a bunch of the logic here. Since pstore is going to need
the init_przs() logic for multiple pmsg przs, I wanted to get this in and
make sure I was happy with how it looks. I figured this would reduce our
round-trip time on reviews. :)
Can you test this series and verify that it works as you're expecting? I've
validated some basic behavior already, but don't have a good test-case for
ftrace. What commands do you actually use for testing ftrace? I'd like to
add something to my local tests.
Thanks!
-Kees
From: Joel Fernandes <[email protected]>
In preparation for merging the per CPU buffers into one buffer when
we retrieve the pstore ftrace data, we store the timestamp as a
counter in the ftrace pstore record. We store the CPU number as well
if !PSTORE_CPU_IN_IP, in this case we shift the counter and may lose
ordering there but we preserve the same record size. The timestamp counter
is also racy, and not doing any locking or synchronization here results
in the benefit of lower overhead. Since we don't care much here for exact
ordering of function traces across CPUs, we don't synchronize and may lose
some counter updates but I'm ok with that.
Using trace_clock() results in much lower performance so avoid using it
since we don't want accuracy in timestamp and need a rough ordering to
perform merge.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: updated commit message, added comments]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ftrace.c | 4 +++
fs/pstore/inode.c | 8 ++++--
fs/pstore/internal.h | 34 ----------------------
include/linux/pstore.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+), 37 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index d4887705bb61..31548cc09e7b 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -27,6 +27,9 @@
#include <asm/barrier.h>
#include "internal.h"
+/* This doesn't need to be atomic: speed is chosen over correctness here. */
+static u64 pstore_ftrace_stamp;
+
static void notrace pstore_ftrace_call(unsigned long ip,
unsigned long parent_ip,
struct ftrace_ops *op,
@@ -42,6 +45,7 @@ static void notrace pstore_ftrace_call(unsigned long ip,
rec.ip = ip;
rec.parent_ip = parent_ip;
+ pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
psinfo->write_buf(PSTORE_TYPE_FTRACE, 0, NULL, 0, (void *)&rec,
0, sizeof(rec), psinfo);
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1781dc50762e..0d6bbcf47d52 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -107,9 +107,11 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
struct pstore_ftrace_seq_data *data = v;
struct pstore_ftrace_record *rec = (void *)(ps->data + data->off);
- seq_printf(s, "%d %08lx %08lx %pf <- %pF\n",
- pstore_ftrace_decode_cpu(rec), rec->ip, rec->parent_ip,
- (void *)rec->ip, (void *)rec->parent_ip);
+ seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
+ pstore_ftrace_decode_cpu(rec),
+ pstore_ftrace_read_timestamp(rec),
+ rec->ip, rec->parent_ip, (void *)rec->ip,
+ (void *)rec->parent_ip);
return 0;
}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index e38a22b31282..da416e6591c9 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -5,40 +5,6 @@
#include <linux/time.h>
#include <linux/pstore.h>
-#if NR_CPUS <= 2 && defined(CONFIG_ARM_THUMB)
-#define PSTORE_CPU_IN_IP 0x1
-#elif NR_CPUS <= 4 && defined(CONFIG_ARM)
-#define PSTORE_CPU_IN_IP 0x3
-#endif
-
-struct pstore_ftrace_record {
- unsigned long ip;
- unsigned long parent_ip;
-#ifndef PSTORE_CPU_IN_IP
- unsigned int cpu;
-#endif
-};
-
-static inline void
-pstore_ftrace_encode_cpu(struct pstore_ftrace_record *rec, unsigned int cpu)
-{
-#ifndef PSTORE_CPU_IN_IP
- rec->cpu = cpu;
-#else
- rec->ip |= cpu;
-#endif
-}
-
-static inline unsigned int
-pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
-{
-#ifndef PSTORE_CPU_IN_IP
- return rec->cpu;
-#else
- return rec->ip & PSTORE_CPU_IN_IP;
-#endif
-}
-
#ifdef CONFIG_PSTORE_FTRACE
extern void pstore_register_ftrace(void);
extern void pstore_unregister_ftrace(void);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 92013cc9cc8c..0da29cae009b 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -89,4 +89,80 @@ extern int pstore_register(struct pstore_info *);
extern void pstore_unregister(struct pstore_info *);
extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
+struct pstore_ftrace_record {
+ unsigned long ip;
+ unsigned long parent_ip;
+ u64 ts;
+};
+
+/*
+ * ftrace related stuff: Both backends and frontends need these so expose
+ * them here.
+ */
+
+#if NR_CPUS <= 2 && defined(CONFIG_ARM_THUMB)
+#define PSTORE_CPU_IN_IP 0x1
+#elif NR_CPUS <= 4 && defined(CONFIG_ARM)
+#define PSTORE_CPU_IN_IP 0x3
+#endif
+
+#define TS_CPU_SHIFT 8
+#define TS_CPU_MASK (BIT(TS_CPU_SHIFT) - 1)
+
+/*
+ * If CPU number can be stored in IP, store it there, otherwise store it in
+ * the time stamp. This means more timestamp resolution is available when
+ * the CPU can be stored in the IP.
+ */
+#ifdef PSTORE_CPU_IN_IP
+static inline void
+pstore_ftrace_encode_cpu(struct pstore_ftrace_record *rec, unsigned int cpu)
+{
+ rec->ip |= cpu;
+}
+
+static inline unsigned int
+pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
+{
+ return rec->ip & PSTORE_CPU_IN_IP;
+}
+
+static inline u64
+pstore_ftrace_read_timestamp(struct pstore_ftrace_record *rec)
+{
+ return rec->ts;
+}
+
+static inline void
+pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
+{
+ rec->ts = val;
+}
+#else
+static inline void
+pstore_ftrace_encode_cpu(struct pstore_ftrace_record *rec, unsigned int cpu)
+{
+ rec->ts &= ~(TS_CPU_MASK);
+ rec->ts |= cpu;
+}
+
+static inline unsigned int
+pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
+{
+ return rec->ts & TS_CPU_MASK;
+}
+
+static inline u64
+pstore_ftrace_read_timestamp(struct pstore_ftrace_record *rec)
+{
+ return rec->ts >> TS_CPU_SHIFT;
+}
+
+static inline void
+pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
+{
+ rec->ts = (rec->ts & TS_CPU_MASK) | (val << TS_CPU_SHIFT);
+}
+#endif
+
#endif /*_LINUX_PSTORE_H*/
--
2.7.4
From: Joel Fernandes <[email protected]>
Currently pstore has a global spinlock for all zones. Since the zones
are independent and modify different areas of memory, there's no need
to have a global lock, so we should use a per-zone lock as introduced
here. Also, when ramoops's ftrace use-case has a FTRACE_PER_CPU flag
introduced later, which splits the ftrace memory area into a single zone
per CPU, it will eliminate the need for locking. In preparation for this,
make the locking optional.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: updated commit message]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram_core.c | 11 +++++------
include/linux/pstore_ram.h | 1 +
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 3975deec02f8..cb92055e6016 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -48,8 +48,6 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
return atomic_read(&prz->buffer->start);
}
-static DEFINE_RAW_SPINLOCK(buffer_lock);
-
/* increase and wrap the start pointer, returning the old value */
static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
{
@@ -57,7 +55,7 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
int new;
unsigned long flags;
- raw_spin_lock_irqsave(&buffer_lock, flags);
+ raw_spin_lock_irqsave(&prz->buffer_lock, flags);
old = atomic_read(&prz->buffer->start);
new = old + a;
@@ -65,7 +63,7 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
new -= prz->buffer_size;
atomic_set(&prz->buffer->start, new);
- raw_spin_unlock_irqrestore(&buffer_lock, flags);
+ raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
return old;
}
@@ -77,7 +75,7 @@ static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
size_t new;
unsigned long flags;
- raw_spin_lock_irqsave(&buffer_lock, flags);
+ raw_spin_lock_irqsave(&prz->buffer_lock, flags);
old = atomic_read(&prz->buffer->size);
if (old == prz->buffer_size)
@@ -89,7 +87,7 @@ static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
atomic_set(&prz->buffer->size, new);
exit:
- raw_spin_unlock_irqrestore(&buffer_lock, flags);
+ raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
}
static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
@@ -493,6 +491,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
prz->buffer->sig = sig;
persistent_ram_zap(prz);
+ prz->buffer_lock = __RAW_SPIN_LOCK_UNLOCKED(buffer_lock);
return 0;
}
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index c668c861c96c..244d2423dbaf 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -40,6 +40,7 @@ struct persistent_ram_zone {
void *vaddr;
struct persistent_ram_buffer *buffer;
size_t buffer_size;
+ raw_spinlock_t buffer_lock;
/* ECC correction */
char *par_buffer;
--
2.7.4
From: Joel Fernandes <[email protected]>
In preparation of not locking at all for certain buffers depending on if
there's contention, make locking optional depending on the initialization
of the prz.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: moved locking flag into prz instead of via caller arguments]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram.c | 5 +++--
fs/pstore/ram_core.c | 20 +++++++++++++-------
include/linux/pstore_ram.h | 10 +++++++++-
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6d1393965b0a..86ac59066fba 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -432,7 +432,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
for (i = 0; i < cxt->max_dump_cnt; i++) {
cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0,
&cxt->ecc_info,
- cxt->memtype);
+ cxt->memtype, 0);
if (IS_ERR(cxt->przs[i])) {
err = PTR_ERR(cxt->przs[i]);
dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
@@ -469,7 +469,8 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
return -ENOMEM;
}
- *prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info, cxt->memtype);
+ *prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
+ cxt->memtype, 0);
if (IS_ERR(*prz)) {
int err = PTR_ERR(*prz);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index cb92055e6016..0cc23cb18719 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -55,7 +55,8 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
int new;
unsigned long flags;
- raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+ if (!(prz->flags & PRZ_FLAG_NO_LOCK))
+ raw_spin_lock_irqsave(&prz->buffer_lock, flags);
old = atomic_read(&prz->buffer->start);
new = old + a;
@@ -63,7 +64,8 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
new -= prz->buffer_size;
atomic_set(&prz->buffer->start, new);
- raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
+ if (!(prz->flags & PRZ_FLAG_NO_LOCK))
+ raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
return old;
}
@@ -75,7 +77,8 @@ static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
size_t new;
unsigned long flags;
- raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+ if (!(prz->flags & PRZ_FLAG_NO_LOCK))
+ raw_spin_lock_irqsave(&prz->buffer_lock, flags);
old = atomic_read(&prz->buffer->size);
if (old == prz->buffer_size)
@@ -87,7 +90,8 @@ static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
atomic_set(&prz->buffer->size, new);
exit:
- raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
+ if (!(prz->flags & PRZ_FLAG_NO_LOCK))
+ raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
}
static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
@@ -463,7 +467,8 @@ static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
}
static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
- struct persistent_ram_ecc_info *ecc_info)
+ struct persistent_ram_ecc_info *ecc_info,
+ unsigned long flags)
{
int ret;
@@ -492,6 +497,7 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
prz->buffer->sig = sig;
persistent_ram_zap(prz);
prz->buffer_lock = __RAW_SPIN_LOCK_UNLOCKED(buffer_lock);
+ prz->flags = flags;
return 0;
}
@@ -516,7 +522,7 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
u32 sig, struct persistent_ram_ecc_info *ecc_info,
- unsigned int memtype)
+ unsigned int memtype, u32 flags)
{
struct persistent_ram_zone *prz;
int ret = -ENOMEM;
@@ -531,7 +537,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
if (ret)
goto err;
- ret = persistent_ram_post_init(prz, sig, ecc_info);
+ ret = persistent_ram_post_init(prz, sig, ecc_info, flags);
if (ret)
goto err;
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 244d2423dbaf..4058bf991868 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -24,6 +24,13 @@
#include <linux/list.h>
#include <linux/types.h>
+/*
+ * Choose whether access to the RAM zone requires locking or not. If a zone
+ * can be written to from different CPUs like with ftrace for example, then
+ * PRZ_FLAG_NO_LOCK is used. For all other cases, locking is required.
+ */
+#define PRZ_FLAG_NO_LOCK BIT(0)
+
struct persistent_ram_buffer;
struct rs_control;
@@ -40,6 +47,7 @@ struct persistent_ram_zone {
void *vaddr;
struct persistent_ram_buffer *buffer;
size_t buffer_size;
+ u32 flags;
raw_spinlock_t buffer_lock;
/* ECC correction */
@@ -56,7 +64,7 @@ struct persistent_ram_zone {
struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
u32 sig, struct persistent_ram_ecc_info *ecc_info,
- unsigned int memtype);
+ unsigned int memtype, u32 flags);
void persistent_ram_free(struct persistent_ram_zone *prz);
void persistent_ram_zap(struct persistent_ram_zone *prz);
--
2.7.4
From: Joel Fernandes <[email protected]>
PMSG now uses ramoops_pstore_write_buf_user() instead of ...write_buf().
Print a ratelimited warning if gets accidentally called.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: adjusted commit log and added -EINVAL return]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6ad831b9d1b8..6d1393965b0a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -288,10 +288,8 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
persistent_ram_write(cxt->fprz, buf, size);
return 0;
} else if (type == PSTORE_TYPE_PMSG) {
- if (!cxt->mprz)
- return -ENOMEM;
- persistent_ram_write(cxt->mprz, buf, size);
- return 0;
+ pr_warn_ratelimited("PMSG shouldn't call %s\n", __func__);
+ return -EINVAL;
}
if (type != PSTORE_TYPE_DMESG)
--
2.7.4
From: Joel Fernandes <[email protected]>
Up until this patch, each of the per CPU ftrace buffers appear as a
separate ftrace-ramoops-N file. In this patch we merge all the zones into
one and populate a single ftrace-ramoops-0 file.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: clarified variables names, added -ENOMEM handling]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 109 insertions(+), 13 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 0ed3fec04c9b..f5d266157964 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -182,16 +182,69 @@ static bool prz_ok(struct persistent_ram_zone *prz)
persistent_ram_ecc_string(prz, NULL, 0));
}
+static ssize_t ftrace_log_combine(struct persistent_ram_zone *dest,
+ struct persistent_ram_zone *src)
+{
+ size_t dest_size, src_size, total, dest_off, src_off;
+ size_t dest_idx = 0, src_idx = 0, merged_idx = 0;
+ void *merged_buf;
+ struct pstore_ftrace_record *drec, *srec, *mrec;
+ size_t record_size = sizeof(struct pstore_ftrace_record);
+
+ dest_off = dest->old_log_size % record_size;
+ dest_size = dest->old_log_size - dest_off;
+
+ src_off = src->old_log_size % record_size;
+ src_size = src->old_log_size - src_off;
+
+ total = dest_size + src_size;
+ merged_buf = kmalloc(total, GFP_KERNEL);
+ if (!merged_buf)
+ return -ENOMEM;
+
+ drec = (struct pstore_ftrace_record *)(dest->old_log + dest_off);
+ srec = (struct pstore_ftrace_record *)(src->old_log + src_off);
+ mrec = (struct pstore_ftrace_record *)(merged_buf);
+
+ while (dest_size > 0 && src_size > 0) {
+ if (pstore_ftrace_read_timestamp(&drec[dest_idx]) <
+ pstore_ftrace_read_timestamp(&srec[src_idx])) {
+ mrec[merged_idx++] = drec[dest_idx++];
+ dest_size -= record_size;
+ } else {
+ mrec[merged_idx++] = srec[src_idx++];
+ src_size -= record_size;
+ }
+ }
+
+ while (dest_size > 0) {
+ mrec[merged_idx++] = drec[dest_idx++];
+ dest_size -= record_size;
+ }
+
+ while (src_size > 0) {
+ mrec[merged_idx++] = srec[src_idx++];
+ src_size -= record_size;
+ }
+
+ kfree(dest->old_log);
+ dest->old_log = merged_buf;
+ dest->old_log_size = total;
+
+ return 0;
+}
+
static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
int *count, struct timespec *time,
char **buf, bool *compressed,
ssize_t *ecc_notice_size,
struct pstore_info *psi)
{
- ssize_t size;
+ ssize_t size = 0;
struct ramoops_context *cxt = psi->data;
struct persistent_ram_zone *prz = NULL;
int header_length = 0;
+ bool free_prz = false;
/* Ramoops headers provide time stamps for PSTORE_TYPE_DMESG, but
* PSTORE_TYPE_CONSOLE and PSTORE_TYPE_FTRACE don't currently have
@@ -221,22 +274,56 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
1, id, type, PSTORE_TYPE_CONSOLE, 0);
+
+ if (!prz_ok(prz))
+ prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
+ 1, id, type, PSTORE_TYPE_PMSG, 0);
+
+ /* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
- while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt && !prz) {
+ if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- &cxt->ftrace_read_cnt,
- cxt->max_ftrace_cnt, id, type,
+ &cxt->ftrace_read_cnt, 1, id, type,
PSTORE_TYPE_FTRACE, 0);
- if (!prz_ok(prz))
- continue;
+ } else {
+ /*
+ * Build a new dummy record which combines all the
+ * per-cpu records including metadata and ecc info.
+ */
+ struct persistent_ram_zone *tmp_prz, *prz_next;
+
+ tmp_prz = kzalloc(sizeof(struct persistent_ram_zone),
+ GFP_KERNEL);
+ if (!tmp_prz)
+ return -ENOMEM;
+ free_prz = true;
+
+ while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
+ prz_next = ramoops_get_next_prz(cxt->fprzs,
+ &cxt->ftrace_read_cnt,
+ cxt->max_ftrace_cnt, id,
+ type, PSTORE_TYPE_FTRACE, 0);
+
+ if (!prz_ok(prz_next))
+ continue;
+
+ tmp_prz->ecc_info = prz_next->ecc_info;
+ tmp_prz->corrected_bytes +=
+ prz_next->corrected_bytes;
+ tmp_prz->bad_blocks += prz_next->bad_blocks;
+ size = ftrace_log_combine(tmp_prz, prz_next);
+ if (size)
+ goto out;
+ }
+ *id = 0;
+ prz = tmp_prz;
}
}
- if (!prz_ok(prz))
- prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
- 1, id, type, PSTORE_TYPE_PMSG, 0);
- if (!prz_ok(prz))
- return 0;
+ if (!prz_ok(prz)) {
+ size = 0;
+ goto out;
+ }
size = persistent_ram_old_size(prz) - header_length;
@@ -244,12 +331,21 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
*ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
*buf = kmalloc(size + *ecc_notice_size + 1, GFP_KERNEL);
- if (*buf == NULL)
- return -ENOMEM;
+ if (*buf == NULL) {
+ size = -ENOMEM;
+ goto out;
+ }
memcpy(*buf, (char *)persistent_ram_old(prz) + header_length, size);
+
persistent_ram_ecc_string(prz, *buf + size, *ecc_notice_size + 1);
+out:
+ if (free_prz) {
+ kfree(prz->old_log);
+ kfree(prz);
+ }
+
return size;
}
--
2.7.4
Currently ramoops_init_przs() is hard wired only for panic dump zone
array. In preparation for the ftrace zone array (one zone per-cpu) and pmsg
zone array, make the function more generic to be able to handle this case.
Heavily based on similar work from Joel Fernandes.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram.c | 82 +++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 28 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 86ac59066fba..34294c32af0b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -405,53 +405,78 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
}
static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
- phys_addr_t *paddr, size_t dump_mem_sz)
+ struct persistent_ram_zone ***przs,
+ phys_addr_t *paddr, size_t mem_sz,
+ ssize_t record_size,
+ unsigned int *cnt, u32 sig, u32 flags)
{
int err = -ENOMEM;
int i;
+ size_t zone_sz;
+ struct persistent_ram_zone **prz_ar;
- if (!cxt->record_size)
+ /* Allocate nothing for 0 mem_sz or 0 record_size. */
+ if (mem_sz == 0 || record_size == 0) {
+ *cnt = 0;
return 0;
-
- if (*paddr + dump_mem_sz - cxt->phys_addr > cxt->size) {
- dev_err(dev, "no room for dumps\n");
- return -ENOMEM;
}
- cxt->max_dump_cnt = dump_mem_sz / cxt->record_size;
- if (!cxt->max_dump_cnt)
- return -ENOMEM;
+ /*
+ * If we have a negative record size, calculate it based on
+ * mem_sz / *cnt. If we have a positive record size, calculate
+ * cnt from mem_sz / record_size.
+ */
+ if (record_size < 0) {
+ if (*cnt == 0)
+ return 0;
+ record_size = mem_sz / *cnt;
+ if (record_size == 0)
+ goto fail;
+ } else {
+ *cnt = mem_sz / record_size;
+ if (*cnt == 0)
+ goto fail;
+ }
- cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_cnt,
- GFP_KERNEL);
- if (!cxt->przs) {
- dev_err(dev, "failed to initialize a prz array for dumps\n");
- goto fail_mem;
+ if (*paddr + mem_sz - cxt->phys_addr > cxt->size) {
+ dev_err(dev, "no room for mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
+ mem_sz, (unsigned long long)*paddr,
+ cxt->size, (unsigned long long)cxt->phys_addr);
+ goto fail;
}
- for (i = 0; i < cxt->max_dump_cnt; i++) {
- cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0,
+ zone_sz = mem_sz / *cnt;
+ if (!zone_sz)
+ goto fail;
+
+ prz_ar = kcalloc(*cnt, sizeof(**przs), GFP_KERNEL);
+ if (!prz_ar)
+ goto fail;
+
+ for (i = 0; i < *cnt; i++) {
+ prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
&cxt->ecc_info,
- cxt->memtype, 0);
- if (IS_ERR(cxt->przs[i])) {
- err = PTR_ERR(cxt->przs[i]);
+ cxt->memtype, flags);
+ if (IS_ERR(prz_ar[i])) {
+ err = PTR_ERR(prz_ar[i]);
dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
- cxt->record_size, (unsigned long long)*paddr, err);
+ record_size, (unsigned long long)*paddr, err);
while (i > 0) {
i--;
- persistent_ram_free(cxt->przs[i]);
+ persistent_ram_free(prz_ar[i]);
}
- goto fail_prz;
+ kfree(prz_ar);
+ goto fail;
}
- *paddr += cxt->record_size;
+ *paddr += zone_sz;
}
+ *przs = prz_ar;
return 0;
-fail_prz:
- kfree(cxt->przs);
-fail_mem:
- cxt->max_dump_cnt = 0;
+
+fail:
+ *cnt = 0;
return err;
}
@@ -605,7 +630,8 @@ static int ramoops_probe(struct platform_device *pdev)
dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
- cxt->pmsg_size;
- err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
+ err = ramoops_init_przs(dev, cxt, &cxt->przs, &paddr, dump_mem_sz,
+ cxt->record_size, &cxt->max_dump_cnt, 0, 0);
if (err)
goto fail_out;
--
2.7.4
From: Joel Fernandes <[email protected]>
If the RAMOOPS_FLAG_FTRACE_PER_CPU flag is passed to ramoops pdata, split
the ftrace space into multiple zones depending on the number of CPUs.
This speeds up the performance of function tracing by about 280% in my
tests as we avoid the locking. The trade off being lesser space available
per CPU. Let the ramoops user decide which option they want based on pdata
flag.
Signed-off-by: Joel Fernandes <[email protected]>
[kees: added max_ftrace_cnt to track size, added DT logic and docs]
Signed-off-by: Kees Cook <[email protected]>
---
.../bindings/reserved-memory/ramoops.txt | 3 +
fs/pstore/ram.c | 72 +++++++++++++++++-----
include/linux/pstore_ram.h | 3 +
3 files changed, 61 insertions(+), 17 deletions(-)
diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index e81f821a2135..0eba562fe5c6 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -46,3 +46,6 @@ Optional properties:
(defaults to buffered mappings)
- no-dump-oops: if present, only dump panics (defaults to panics and oops)
+
+- flags: if present, pass ramoops behavioral flags (defaults to 0,
+ see include/linux/pstore_ram.h RAMOOPS_FLAG_* for flag values).
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 34294c32af0b..0ed3fec04c9b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -87,7 +87,7 @@ MODULE_PARM_DESC(ramoops_ecc,
struct ramoops_context {
struct persistent_ram_zone **przs;
struct persistent_ram_zone *cprz;
- struct persistent_ram_zone *fprz;
+ struct persistent_ram_zone **fprzs;
struct persistent_ram_zone *mprz;
phys_addr_t phys_addr;
unsigned long size;
@@ -97,12 +97,14 @@ struct ramoops_context {
size_t ftrace_size;
size_t pmsg_size;
int dump_oops;
+ u32 flags;
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
unsigned int dump_write_cnt;
/* _read_cnt need clear on ramoops_pstore_open */
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
+ unsigned int max_ftrace_cnt;
unsigned int ftrace_read_cnt;
unsigned int pmsg_read_cnt;
struct pstore_info pstore;
@@ -219,9 +221,17 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
1, id, type, PSTORE_TYPE_CONSOLE, 0);
- if (!prz_ok(prz))
- prz = ramoops_get_next_prz(&cxt->fprz, &cxt->ftrace_read_cnt,
- 1, id, type, PSTORE_TYPE_FTRACE, 0);
+ if (!prz_ok(prz)) {
+ while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt && !prz) {
+ prz = ramoops_get_next_prz(cxt->fprzs,
+ &cxt->ftrace_read_cnt,
+ cxt->max_ftrace_cnt, id, type,
+ PSTORE_TYPE_FTRACE, 0);
+ if (!prz_ok(prz))
+ continue;
+ }
+ }
+
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
1, id, type, PSTORE_TYPE_PMSG, 0);
@@ -283,9 +293,19 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
persistent_ram_write(cxt->cprz, buf, size);
return 0;
} else if (type == PSTORE_TYPE_FTRACE) {
- if (!cxt->fprz)
+ int zonenum;
+
+ if (!cxt->fprzs)
return -ENOMEM;
- persistent_ram_write(cxt->fprz, buf, size);
+ /*
+ * Choose zone by if we're using per-cpu buffers.
+ */
+ if (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
+ zonenum = smp_processor_id();
+ else
+ zonenum = 0;
+
+ persistent_ram_write(cxt->fprzs[zonenum], buf, size);
return 0;
} else if (type == PSTORE_TYPE_PMSG) {
pr_warn_ratelimited("PMSG shouldn't call %s\n", __func__);
@@ -363,7 +383,9 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
prz = cxt->cprz;
break;
case PSTORE_TYPE_FTRACE:
- prz = cxt->fprz;
+ if (id >= cxt->max_ftrace_cnt)
+ return -EINVAL;
+ prz = cxt->fprzs[id];
break;
case PSTORE_TYPE_PMSG:
prz = cxt->mprz;
@@ -394,14 +416,22 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
{
int i;
- if (!cxt->przs)
- return;
+ /* Free dump PRZs */
+ if (cxt->przs) {
+ for (i = 0; i < cxt->max_dump_cnt; i++)
+ persistent_ram_free(cxt->przs[i]);
- for (i = 0; i < cxt->max_dump_cnt; i++)
- persistent_ram_free(cxt->przs[i]);
+ kfree(cxt->przs);
+ cxt->max_dump_cnt = 0;
+ }
- kfree(cxt->przs);
- cxt->max_dump_cnt = 0;
+ /* Free ftrace PRZs */
+ if (cxt->fprzs) {
+ for (i = 0; i < cxt->max_ftrace_cnt; i++)
+ persistent_ram_free(cxt->fprzs[i]);
+ kfree(cxt->fprzs);
+ cxt->max_ftrace_cnt = 0;
+ }
}
static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
@@ -567,6 +597,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
parse_size("ftrace-size", pdata->ftrace_size);
parse_size("pmsg-size", pdata->pmsg_size);
parse_size("ecc-size", pdata->ecc_info.ecc_size);
+ parse_size("flags", pdata->flags);
#undef parse_size
@@ -624,6 +655,7 @@ static int ramoops_probe(struct platform_device *pdev)
cxt->ftrace_size = pdata->ftrace_size;
cxt->pmsg_size = pdata->pmsg_size;
cxt->dump_oops = pdata->dump_oops;
+ cxt->flags = pdata->flags;
cxt->ecc_info = pdata->ecc_info;
paddr = cxt->phys_addr;
@@ -640,8 +672,14 @@ static int ramoops_probe(struct platform_device *pdev)
if (err)
goto fail_init_cprz;
- err = ramoops_init_prz(dev, cxt, &cxt->fprz, &paddr, cxt->ftrace_size,
- LINUX_VERSION_CODE);
+ cxt->max_ftrace_cnt = (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
+ ? nr_cpu_ids
+ : 1;
+ err = ramoops_init_przs(dev, cxt, &cxt->fprzs, &paddr, cxt->ftrace_size,
+ -1, &cxt->max_ftrace_cnt,
+ LINUX_VERSION_CODE,
+ (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
+ ? PRZ_FLAG_NO_LOCK : 0);
if (err)
goto fail_init_fprz;
@@ -705,7 +743,6 @@ static int ramoops_probe(struct platform_device *pdev)
cxt->pstore.bufsize = 0;
persistent_ram_free(cxt->mprz);
fail_init_mprz:
- persistent_ram_free(cxt->fprz);
fail_init_fprz:
persistent_ram_free(cxt->cprz);
fail_init_cprz:
@@ -724,7 +761,6 @@ static int ramoops_remove(struct platform_device *pdev)
cxt->pstore.bufsize = 0;
persistent_ram_free(cxt->mprz);
- persistent_ram_free(cxt->fprz);
persistent_ram_free(cxt->cprz);
ramoops_free_przs(cxt);
@@ -766,6 +802,8 @@ static void ramoops_register_dummy(void)
dummy_data->ftrace_size = ramoops_ftrace_size;
dummy_data->pmsg_size = ramoops_pmsg_size;
dummy_data->dump_oops = dump_oops;
+ dummy_data->flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
+
/*
* For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
* (using 1 byte for ECC isn't much of use anyway).
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 4058bf991868..9395f06e8372 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -86,6 +86,8 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
* @mem_address physical memory address to contain ramoops
*/
+#define RAMOOPS_FLAG_FTRACE_PER_CPU BIT(0)
+
struct ramoops_platform_data {
unsigned long mem_size;
phys_addr_t mem_address;
@@ -95,6 +97,7 @@ struct ramoops_platform_data {
unsigned long ftrace_size;
unsigned long pmsg_size;
int dump_oops;
+ u32 flags;
struct persistent_ram_ecc_info ecc_info;
};
--
2.7.4
When setting ramoops record sizes, sometimes it's not clear which
parameters contributed to the allocation failure. This adds a per-zone
name and expands the failure reports.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ram.c | 53 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 19 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index f5d266157964..64fd6ead82cc 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -530,7 +530,8 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
}
}
-static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
+static int ramoops_init_przs(const char *name,
+ struct device *dev, struct ramoops_context *cxt,
struct persistent_ram_zone ***przs,
phys_addr_t *paddr, size_t mem_sz,
ssize_t record_size,
@@ -556,24 +557,33 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
if (*cnt == 0)
return 0;
record_size = mem_sz / *cnt;
- if (record_size == 0)
+ if (record_size == 0) {
+ dev_err(dev, "%s record size == 0 (%zu / %u)\n",
+ name, mem_sz, *cnt);
goto fail;
+ }
} else {
*cnt = mem_sz / record_size;
- if (*cnt == 0)
+ if (*cnt == 0) {
+ dev_err(dev, "%s record count == 0 (%zu / %zu)\n",
+ name, mem_sz, record_size);
goto fail;
+ }
}
if (*paddr + mem_sz - cxt->phys_addr > cxt->size) {
- dev_err(dev, "no room for mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
+ dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
+ name,
mem_sz, (unsigned long long)*paddr,
cxt->size, (unsigned long long)cxt->phys_addr);
goto fail;
}
zone_sz = mem_sz / *cnt;
- if (!zone_sz)
+ if (!zone_sz) {
+ dev_err(dev, "%s zone size == 0\n", name);
goto fail;
+ }
prz_ar = kcalloc(*cnt, sizeof(**przs), GFP_KERNEL);
if (!prz_ar)
@@ -585,8 +595,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
cxt->memtype, flags);
if (IS_ERR(prz_ar[i])) {
err = PTR_ERR(prz_ar[i]);
- dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
- record_size, (unsigned long long)*paddr, err);
+ dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
+ name, record_size,
+ (unsigned long long)*paddr, err);
while (i > 0) {
i--;
@@ -606,7 +617,8 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
return err;
}
-static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
+static int ramoops_init_prz(const char *name,
+ struct device *dev, struct ramoops_context *cxt,
struct persistent_ram_zone **prz,
phys_addr_t *paddr, size_t sz, u32 sig)
{
@@ -614,8 +626,8 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
return 0;
if (*paddr + sz - cxt->phys_addr > cxt->size) {
- dev_err(dev, "no room for mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
- sz, (unsigned long long)*paddr,
+ dev_err(dev, "no room for %s mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n",
+ name, sz, (unsigned long long)*paddr,
cxt->size, (unsigned long long)cxt->phys_addr);
return -ENOMEM;
}
@@ -625,8 +637,8 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
if (IS_ERR(*prz)) {
int err = PTR_ERR(*prz);
- dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
- sz, (unsigned long long)*paddr, err);
+ dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
+ name, sz, (unsigned long long)*paddr, err);
return err;
}
@@ -712,6 +724,7 @@ static int ramoops_probe(struct platform_device *pdev)
if (dev_of_node(dev) && !pdata) {
pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
+ pr_err("cannot allocate platform data buffer\n");
err = -ENOMEM;
goto fail_out;
}
@@ -758,12 +771,13 @@ static int ramoops_probe(struct platform_device *pdev)
dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
- cxt->pmsg_size;
- err = ramoops_init_przs(dev, cxt, &cxt->przs, &paddr, dump_mem_sz,
- cxt->record_size, &cxt->max_dump_cnt, 0, 0);
+ err = ramoops_init_przs("dump", dev, cxt, &cxt->przs, &paddr,
+ dump_mem_sz, cxt->record_size,
+ &cxt->max_dump_cnt, 0, 0);
if (err)
goto fail_out;
- err = ramoops_init_prz(dev, cxt, &cxt->cprz, &paddr,
+ err = ramoops_init_prz("console", dev, cxt, &cxt->cprz, &paddr,
cxt->console_size, 0);
if (err)
goto fail_init_cprz;
@@ -771,15 +785,16 @@ static int ramoops_probe(struct platform_device *pdev)
cxt->max_ftrace_cnt = (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
? nr_cpu_ids
: 1;
- err = ramoops_init_przs(dev, cxt, &cxt->fprzs, &paddr, cxt->ftrace_size,
- -1, &cxt->max_ftrace_cnt,
- LINUX_VERSION_CODE,
+ err = ramoops_init_przs("ftrace", dev, cxt, &cxt->fprzs, &paddr,
+ cxt->ftrace_size, -1,
+ &cxt->max_ftrace_cnt, LINUX_VERSION_CODE,
(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
? PRZ_FLAG_NO_LOCK : 0);
if (err)
goto fail_init_fprz;
- err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr, cxt->pmsg_size, 0);
+ err = ramoops_init_prz("pmsg", dev, cxt, &cxt->mprz, &paddr,
+ cxt->pmsg_size, 0);
if (err)
goto fail_init_mprz;
--
2.7.4
Hi Kees,
On Fri, Nov 11, 2016 at 2:21 PM, Kees Cook <[email protected]> wrote:
> Hi Joel,
>
> I've reorganized a bunch of the logic here. Since pstore is going to need
> the init_przs() logic for multiple pmsg przs, I wanted to get this in and
> make sure I was happy with how it looks. I figured this would reduce our
> round-trip time on reviews. :)
>
> Can you test this series and verify that it works as you're expecting? I've
> validated some basic behavior already, but don't have a good test-case for
> ftrace. What commands do you actually use for testing ftrace? I'd like to
> add something to my local tests.
I normally do the following:
dd if=/dev/urandom | pv | dd of=/dev/null
and in parallel, I do a:
echo 1 > /sys/kernel/debug/pstore/record_ftrace
and then check the throughput; and then reboot the system and do a
read out of /sys/fs/pstore/ ftrace file.
I will try these patches out today and thanks for refactoring the init
prz stuff.
Regards,
Joel
On Tue, Nov 15, 2016 at 11:55 AM, Joel Fernandes <[email protected]> wrote:
> Hi Kees,
>
> On Fri, Nov 11, 2016 at 2:21 PM, Kees Cook <[email protected]> wrote:
>> Hi Joel,
>>
>> I've reorganized a bunch of the logic here. Since pstore is going to need
>> the init_przs() logic for multiple pmsg przs, I wanted to get this in and
>> make sure I was happy with how it looks. I figured this would reduce our
>> round-trip time on reviews. :)
>>
>> Can you test this series and verify that it works as you're expecting? I've
>> validated some basic behavior already, but don't have a good test-case for
>> ftrace. What commands do you actually use for testing ftrace? I'd like to
>> add something to my local tests.
>
> I normally do the following:
>
> dd if=/dev/urandom | pv | dd of=/dev/null
>
> and in parallel, I do a:
> echo 1 > /sys/kernel/debug/pstore/record_ftrace
>
> and then check the throughput; and then reboot the system and do a
> read out of /sys/fs/pstore/ ftrace file.
Cool. Does something normally parse these? Lots of kernel addresses is
all I see. ;)
> I will try these patches out today and thanks for refactoring the init
> prz stuff.
Sure thing! It should make the multi-pmsg change easier too.
-Kees
--
Kees Cook
Nexus Security
Hi Kees,
On Tue, Nov 15, 2016 at 1:36 PM, Kees Cook <[email protected]> wrote:
> On Tue, Nov 15, 2016 at 11:55 AM, Joel Fernandes <[email protected]> wrote:
>> Hi Kees,
>>
>> On Fri, Nov 11, 2016 at 2:21 PM, Kees Cook <[email protected]> wrote:
>>> Hi Joel,
>>>
>>> I've reorganized a bunch of the logic here. Since pstore is going to need
>>> the init_przs() logic for multiple pmsg przs, I wanted to get this in and
>>> make sure I was happy with how it looks. I figured this would reduce our
>>> round-trip time on reviews. :)
>>>
>>> Can you test this series and verify that it works as you're expecting? I've
>>> validated some basic behavior already, but don't have a good test-case for
>>> ftrace. What commands do you actually use for testing ftrace? I'd like to
>>> add something to my local tests.
>>
>> I normally do the following:
>>
>> dd if=/dev/urandom | pv | dd of=/dev/null
>>
>> and in parallel, I do a:
>> echo 1 > /sys/kernel/debug/pstore/record_ftrace
>>
>> and then check the throughput; and then reboot the system and do a
>> read out of /sys/fs/pstore/ ftrace file.
>
> Cool. Does something normally parse these? Lots of kernel addresses is
> all I see. ;)
>
It should print symbol names if KALLSYMS is working properly as it
uses %pf (in pstore_ftrace_seq_show function).
Thanks,
Joel
On Tue, Nov 15, 2016 at 2:06 PM, Joel Fernandes <[email protected]> wrote:
> Hi Kees,
>
> On Tue, Nov 15, 2016 at 1:36 PM, Kees Cook <[email protected]> wrote:
>> On Tue, Nov 15, 2016 at 11:55 AM, Joel Fernandes <[email protected]> wrote:
>>> Hi Kees,
>>>
>>> On Fri, Nov 11, 2016 at 2:21 PM, Kees Cook <[email protected]> wrote:
>>>> Hi Joel,
>>>>
>>>> I've reorganized a bunch of the logic here. Since pstore is going to need
>>>> the init_przs() logic for multiple pmsg przs, I wanted to get this in and
>>>> make sure I was happy with how it looks. I figured this would reduce our
>>>> round-trip time on reviews. :)
>>>>
>>>> Can you test this series and verify that it works as you're expecting? I've
>>>> validated some basic behavior already, but don't have a good test-case for
>>>> ftrace. What commands do you actually use for testing ftrace? I'd like to
>>>> add something to my local tests.
>>>
>>> I normally do the following:
>>>
>>> dd if=/dev/urandom | pv | dd of=/dev/null
>>>
>>> and in parallel, I do a:
>>> echo 1 > /sys/kernel/debug/pstore/record_ftrace
>>>
>>> and then check the throughput; and then reboot the system and do a
>>> read out of /sys/fs/pstore/ ftrace file.
>>
>> Cool. Does something normally parse these? Lots of kernel addresses is
>> all I see. ;)
>>
>
> It should print symbol names if KALLSYMS is working properly as it
> uses %pf (in pstore_ftrace_seq_show function).
Hrm. No such luck for me, but it's clearly using pstore correctly, so
I'm satisfied things are working along that path. :)
-Kees
--
Kees Cook
Nexus Security
On Tue, Nov 15, 2016 at 2:14 PM, Kees Cook <[email protected]> wrote:
> On Tue, Nov 15, 2016 at 2:06 PM, Joel Fernandes <[email protected]> wrote:
>> Hi Kees,
>>
>> On Tue, Nov 15, 2016 at 1:36 PM, Kees Cook <[email protected]> wrote:
>>> On Tue, Nov 15, 2016 at 11:55 AM, Joel Fernandes <[email protected]> wrote:
>>>> Hi Kees,
>>>>
>>>> On Fri, Nov 11, 2016 at 2:21 PM, Kees Cook <[email protected]> wrote:
>>>>> Hi Joel,
>>>>>
>>>>> I've reorganized a bunch of the logic here. Since pstore is going to need
>>>>> the init_przs() logic for multiple pmsg przs, I wanted to get this in and
>>>>> make sure I was happy with how it looks. I figured this would reduce our
>>>>> round-trip time on reviews. :)
>>>>>
>>>>> Can you test this series and verify that it works as you're expecting? I've
>>>>> validated some basic behavior already, but don't have a good test-case for
>>>>> ftrace. What commands do you actually use for testing ftrace? I'd like to
>>>>> add something to my local tests.
>>>>
>>>> I normally do the following:
>>>>
>>>> dd if=/dev/urandom | pv | dd of=/dev/null
>>>>
>>>> and in parallel, I do a:
>>>> echo 1 > /sys/kernel/debug/pstore/record_ftrace
>>>>
>>>> and then check the throughput; and then reboot the system and do a
>>>> read out of /sys/fs/pstore/ ftrace file.
>>>
>>> Cool. Does something normally parse these? Lots of kernel addresses is
>>> all I see. ;)
>>>
>>
>> It should print symbol names if KALLSYMS is working properly as it
>> uses %pf (in pstore_ftrace_seq_show function).
>
> Hrm. No such luck for me, but it's clearly using pstore correctly, so
> I'm satisfied things are working along that path. :)
Tested your for-next/pstore branch and ftrace on pstore works fine.
Thanks!
Joel