pstore: Improve performance of ftrace backend with ramoops

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, ramoops's ftrace usecase has a FTRACE_PER_CPU flag introduced in this patch series which splits the ftrace memory area into a single zone per CPU thus eliminating the need for locking. In preparation for this, make the locking optional.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram_core.c       | 11 +++++------
 include/linux/pstore_ram.h |  1 +
 2 files changed, 6 insertions(+), 6 deletions(-)

fs/pstore/ram_core.c +5 -6

index 3975dee..cb92055 100644
@@ -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;
 }

include/linux/pstore_ram.h +1 -1

index c668c86..244d242 100644
@@ -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

Quoted 81 lines · Joel Fernandes

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, ramoops's ftrace usecase has a FTRACE_PER_CPU flag introduced in this patch series which splits the ftrace memory area into a single zone per CPU thus eliminating the need for locking. In preparation for this, make the locking optional.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram_core.c       | 11 +++++------
 include/linux/pstore_ram.h |  1 +
 2 files changed, 6 insertions(+), 6 deletions(-)

fs/pstore/ram_core.c +5 -6

index 3975dee..cb92055 100644
@@ -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;
 }

include/linux/pstore_ram.h +1 -0

index c668c86..244d242 100644
@@ -40,6 +40,7 @@ struct persistent_ram_zone {
        void *vaddr;
        struct persistent_ram_buffer *buffer;
        size_t buffer_size;
+       raw_spinlock_t buffer_lock;
 

Quoted 5 lines

include/linux/pstore_ram.h +0 -1

        /* ECC correction */
        char *par_buffer;
--

2.7.4

This one looks good, thanks. I'll get this in for -next since it's a good clean-up on its own.

Have you put these patches through scripts/checkpatch.pl? Some of lines wrap or have wide commit messages, but that's just cosmetic to fix up.

In preparation of not locking at all for certain buffers depending on if there's contention, make locking optional depending if caller requested it.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram.c            | 10 +++++-----
 fs/pstore/ram_core.c       | 27 ++++++++++++++++-----------
 include/linux/pstore_ram.h | 10 +++++++++-
 3 files changed, 30 insertions(+), 17 deletions(-)

fs/pstore/ram.c +5 -5

index f1219af..cb07ef6 100644
@@ -260,7 +260,7 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
                compressed ? 'C' : 'D');
        WARN_ON_ONCE(!hdr);
        len = hdr ? strlen(hdr) : 0;
-       persistent_ram_write(prz, hdr, len);
+       persistent_ram_write(prz, hdr, len, PSTORE_RAM_LOCK);
        kfree(hdr);
 
        return len;

@@ -280,17 +280,17 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
        if (type == PSTORE_TYPE_CONSOLE) {
                if (!cxt->cprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->cprz, buf, size);
+               persistent_ram_write(cxt->cprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_FTRACE) {
                if (!cxt->fprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->fprz, buf, size);
+               persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
                if (!cxt->mprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->mprz, buf, size);
+               persistent_ram_write(cxt->mprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        }
 

@@ -324,7 +324,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
        hlen = ramoops_write_kmsg_hdr(prz, compressed);
        if (size + hlen > prz->buffer_size)
                size = prz->buffer_size - hlen;
-       persistent_ram_write(prz, buf, size);
+       persistent_ram_write(prz, buf, size, PSTORE_RAM_LOCK);
 
        cxt->dump_write_cnt = (cxt->dump_write_cnt + 1) % cxt->max_dump_cnt;
 

fs/pstore/ram_core.c +9 -5

index cb92055..69c7b96 100644
@@ -49,13 +49,15 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
 }
 
 /* increase and wrap the start pointer, returning the old value */
-static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
+static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a,
+                              int lock)
 {
        int old;
        int new;
        unsigned long flags;
 
-       raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+       if (lock == PSTORE_RAM_LOCK)
+               raw_spin_lock_irqsave(&prz->buffer_lock, flags);
 
        old = atomic_read(&prz->buffer->start);
        new = old + a;

@@ -63,19 +65,21 @@ 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 (lock == PSTORE_RAM_LOCK)
+               raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
 
        return old;
 }
 
 /* increase the size counter until it hits the max size */
-static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
+static void buffer_size_add(struct persistent_ram_zone *prz, size_t a, int lock)
 {
        size_t old;
        size_t new;
        unsigned long flags;
 
-       raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+       if (lock == PSTORE_RAM_LOCK)
+               raw_spin_lock_irqsave(&prz->buffer_lock, flags);
 
        old = atomic_read(&prz->buffer->size);
        if (old == prz->buffer_size)

Quoted 101 lines · Joel Fernandes

In preparation of not locking at all for certain buffers depending on if there's contention, make locking optional depending if caller requested it.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram.c            | 10 +++++-----
 fs/pstore/ram_core.c       | 27 ++++++++++++++++-----------
 include/linux/pstore_ram.h | 10 +++++++++-
 3 files changed, 30 insertions(+), 17 deletions(-)

fs/pstore/ram.c +5 -5

index f1219af..cb07ef6 100644
@@ -260,7 +260,7 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
                compressed ? 'C' : 'D');
        WARN_ON_ONCE(!hdr);
        len = hdr ? strlen(hdr) : 0;
-       persistent_ram_write(prz, hdr, len);
+       persistent_ram_write(prz, hdr, len, PSTORE_RAM_LOCK);
        kfree(hdr);
 
        return len;

@@ -280,17 +280,17 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
        if (type == PSTORE_TYPE_CONSOLE) {
                if (!cxt->cprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->cprz, buf, size);
+               persistent_ram_write(cxt->cprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_FTRACE) {
                if (!cxt->fprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->fprz, buf, size);
+               persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
                if (!cxt->mprz)
                        return -ENOMEM;
-               persistent_ram_write(cxt->mprz, buf, size);
+               persistent_ram_write(cxt->mprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        }
 

@@ -324,7 +324,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
        hlen = ramoops_write_kmsg_hdr(prz, compressed);
        if (size + hlen > prz->buffer_size)
                size = prz->buffer_size - hlen;
-       persistent_ram_write(prz, buf, size);
+       persistent_ram_write(prz, buf, size, PSTORE_RAM_LOCK);
 
        cxt->dump_write_cnt = (cxt->dump_write_cnt + 1) % cxt->max_dump_cnt;
 

fs/pstore/ram_core.c +9 -5

index cb92055..69c7b96 100644
@@ -49,13 +49,15 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
 }
 
 /* increase and wrap the start pointer, returning the old value */
-static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
+static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a,
+                              int lock)
 {
        int old;
        int new;
        unsigned long flags;
 
-       raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+       if (lock == PSTORE_RAM_LOCK)
+               raw_spin_lock_irqsave(&prz->buffer_lock, flags);
 
        old = atomic_read(&prz->buffer->start);
        new = old + a;

@@ -63,19 +65,21 @@ 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 (lock == PSTORE_RAM_LOCK)
+               raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
 
        return old;
 }
 
 /* increase the size counter until it hits the max size */
-static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
+static void buffer_size_add(struct persistent_ram_zone *prz, size_t a, int lock)
 {
        size_t old;
        size_t new;
        unsigned long flags;
 
-       raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+       if (lock == PSTORE_RAM_LOCK)
+               raw_spin_lock_irqsave(&prz->buffer_lock, flags);
 
        old = atomic_read(&prz->buffer->size);
        if (old == prz->buffer_size)

fs/pstore/ram_core.c static void buffer_size_add(struct persistent_ram_zone *prz, size_t a) · line 98

Quoted 9 lines

fs/pstore/ram_core.c +2 -1

@@ -87,7 +91,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 (lock == PSTORE_RAM_LOCK)
+               raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
 }
 
 static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,

Does this pass a sparse test? Depending on how dumb sparse is, you may need to add "else" statements to the lock == RAM_LOCK tests:

else

        __acquire(&prz->buffer_lock);

...

else

        __release(&prz->buffer_lock);

Otherwise, this looks fine. :)

PMSG now uses ramoops_pstore_write_buf_user instead of ramoops_pstore_write_buf Print a ratelimited warning if the ramoops_pstore_write_buf is called.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/pstore/ram.c +2 -5

index cb07ef6..9104ae9 100644
@@ -288,10 +288,8 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
                persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
-               if (!cxt->mprz)
-                       return -ENOMEM;
-               persistent_ram_write(cxt->mprz, buf, size, PSTORE_RAM_LOCK);
-               return 0;
+               pr_warn_ratelimited("ramoops: warning: PMSG shouldn't call %s\n",
+                                   __func__);
        }
 
        if (type != PSTORE_TYPE_DMESG)
--

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), make the function more generic to be able to handle this case.

Add a new ramoops_init_dump_przs function and use the generic function for the dump prz array.

Signed-off-by Joel Fernandes <[email protected]>

---
 fs/pstore/ram.c | 72 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 26 deletions(-)

fs/pstore/ram.c +46 -27

index 9104ae9..5caa512 100644
@@ -405,54 +405,74 @@ 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,
+                            unsigned int cnt, u32 sig)
 {
        int err = -ENOMEM;
        int i;
+       size_t zone_sz;
+       struct persistent_ram_zone **prz_ar;
 
-       if (!cxt->record_size)
-               return 0;
-
-       if (*paddr + dump_mem_sz - cxt->phys_addr > cxt->size) {
-               dev_err(dev, "no room for dumps\n");
+       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);
                return -ENOMEM;
        }
 
-       cxt->max_dump_cnt = dump_mem_sz / cxt->record_size;
-       if (!cxt->max_dump_cnt)
+       zone_sz = mem_sz / cnt;
+       if (!zone_sz)
                return -ENOMEM;
 
-       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;
+       prz_ar = kcalloc(cnt, sizeof(**przs), GFP_KERNEL);
+       if (!prz_ar) {
+               dev_err(dev, "Failed to initialize prz array\n");
+               return -ENOMEM;
        }
 
-       for (i = 0; i < cxt->max_dump_cnt; i++) {
-               cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0,
+       for (i = 0; i < cnt; i++) {
+               prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
                                                  &cxt->ecc_info,
                                                  cxt->memtype);
-               if (IS_ERR(cxt->przs[i])) {
-                       err = PTR_ERR(cxt->przs[i]);
+               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);
 
                        while (i > 0) {
                                i--;
-                               persistent_ram_free(cxt->przs[i]);
+                               persistent_ram_free(prz_ar[i]);
                        }
-                       goto fail_prz;
+                       kfree(prz_ar);
+                       return err;
                }
-               *paddr += cxt->record_size;
+               *paddr += zone_sz;
        }
 
+       *przs = prz_ar;
        return 0;
-fail_prz:
-       kfree(cxt->przs);
-fail_mem:
-       cxt->max_dump_cnt = 0;
-       return err;
+}
+
+static int ramoops_init_dump_przs(struct device *dev,
+                                 struct ramoops_context *cxt,
+                                 phys_addr_t *paddr, size_t mem_sz)
+{
+       int ret;
+
+       if (!cxt->record_size)
+               return 0;
+
+       cxt->max_dump_cnt = mem_sz / cxt->record_size;
+       if (!cxt->max_dump_cnt)
+               return -ENOMEM;
+
+       ret = ramoops_init_przs(dev, cxt, &cxt->przs, paddr, mem_sz,
+                               cxt->max_dump_cnt, 0);
+       if (ret)
+               cxt->max_dump_cnt = 0;
+
+       return ret;
 }
 
 static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,

@@ -604,7 +624,7 @@ 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_dump_przs(dev, cxt, &paddr, dump_mem_sz);
        if (err)
                goto fail_out;
 
--

2.7.4

If FTRACE_PER_CPU flag is passed to ramoops pdata, split the 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]>

---
 fs/pstore/ram.c            | 72 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 58 insertions(+), 17 deletions(-)

fs/pstore/ram.c +55 -17

index 5caa512..5e96f78 100644
@@ -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,6 +97,7 @@ struct ramoops_context {
        size_t ftrace_size;
        size_t pmsg_size;
        int dump_oops;
+       int flags;
        struct persistent_ram_ecc_info ecc_info;
        unsigned int max_dump_cnt;
        unsigned int dump_write_cnt;

@@ -219,9 +220,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)) {
+               int max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               while (cxt->ftrace_read_cnt < max && !prz) {
+                       prz = ramoops_get_next_prz(cxt->fprzs,
+                                       &cxt->ftrace_read_cnt, max, 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 +292,23 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
                persistent_ram_write(cxt->cprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_FTRACE) {
-               if (!cxt->fprz)
+               int zonenum, lock;
+
+               if (!cxt->fprzs)
                        return -ENOMEM;
-               persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
+               /*
+                * If per-cpu buffers, don't lock. Otherwise there's only
+                * 1 zone for ftrace (zone 0) and all CPUs share it, so lock.
+                */
+               if (cxt->flags & FTRACE_PER_CPU) {
+                       zonenum = smp_processor_id();
+                       lock = PSTORE_RAM_NOLOCK;
+               } else {
+                       zonenum = 0;
+                       lock = PSTORE_RAM_LOCK;
+               }
+
+               persistent_ram_write(cxt->fprzs[zonenum], buf, size, lock);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
                pr_warn_ratelimited("ramoops: warning: PMSG shouldn't call %s\n",

@@ -352,6 +375,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
 {
        struct ramoops_context *cxt = psi->data;
        struct persistent_ram_zone *prz;
+       int max;
 
        switch (type) {
        case PSTORE_TYPE_DMESG:

@@ -363,7 +387,10 @@ 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;
+               max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               if (id >= max)
+                       return -EINVAL;
+               prz = cxt->fprzs[id];
                break;
        case PSTORE_TYPE_PMSG:
                prz = cxt->mprz;

@@ -394,14 +421,23 @@ 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) {
+               int nr = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+
+               for (i = 0; i < nr; i++)
+                       persistent_ram_free(cxt->fprzs[i]);
+               kfree(cxt->fprzs);
+       }
 }
 
 static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,

@@ -618,6 +654,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;

@@ -633,8 +670,9 @@ 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);
+       err = ramoops_init_przs(dev, cxt, &cxt->fprzs, &paddr, cxt->ftrace_size,
+                               (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1,
+                               LINUX_VERSION_CODE);
        if (err)
                goto fail_init_fprz;
 

@@ -698,7 +736,6 @@ fail_clear:
        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:

@@ -717,7 +754,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);
 

@@ -759,6 +795,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 = FTRACE_PER_CPU;
+
        /*
         * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
         * (using 1 byte for ECC isn't much of use anyway).

include/linux/pstore_ram.h +3 -1

index 69f3487..122f78d 100644
@@ -86,6 +86,8 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
  * @mem_address        physical memory address to contain ramoops
  */
 
+#define 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;
+       int             flags;
        struct persistent_ram_ecc_info ecc_info;
 };
 
--

2.7.4

Quoted 197 lines · Joel Fernandes

If FTRACE_PER_CPU flag is passed to ramoops pdata, split the 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]>

---
 fs/pstore/ram.c            | 72 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 58 insertions(+), 17 deletions(-)

fs/pstore/ram.c +55 -17

index 5caa512..5e96f78 100644
@@ -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,6 +97,7 @@ struct ramoops_context {
        size_t ftrace_size;
        size_t pmsg_size;
        int dump_oops;
+       int flags;
        struct persistent_ram_ecc_info ecc_info;
        unsigned int max_dump_cnt;
        unsigned int dump_write_cnt;

@@ -219,9 +220,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)) {
+               int max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               while (cxt->ftrace_read_cnt < max && !prz) {
+                       prz = ramoops_get_next_prz(cxt->fprzs,
+                                       &cxt->ftrace_read_cnt, max, 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 +292,23 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
                persistent_ram_write(cxt->cprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_FTRACE) {
-               if (!cxt->fprz)
+               int zonenum, lock;
+
+               if (!cxt->fprzs)
                        return -ENOMEM;
-               persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
+               /*
+                * If per-cpu buffers, don't lock. Otherwise there's only
+                * 1 zone for ftrace (zone 0) and all CPUs share it, so lock.
+                */
+               if (cxt->flags & FTRACE_PER_CPU) {
+                       zonenum = smp_processor_id();
+                       lock = PSTORE_RAM_NOLOCK;
+               } else {
+                       zonenum = 0;
+                       lock = PSTORE_RAM_LOCK;
+               }
+
+               persistent_ram_write(cxt->fprzs[zonenum], buf, size, lock);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
                pr_warn_ratelimited("ramoops: warning: PMSG shouldn't call %s\n",

@@ -352,6 +375,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
 {
        struct ramoops_context *cxt = psi->data;
        struct persistent_ram_zone *prz;
+       int max;
 
        switch (type) {
        case PSTORE_TYPE_DMESG:

@@ -363,7 +387,10 @@ 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;
+               max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               if (id >= max)
+                       return -EINVAL;
+               prz = cxt->fprzs[id];
                break;
        case PSTORE_TYPE_PMSG:
                prz = cxt->mprz;

@@ -394,14 +421,23 @@ 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) {
+               int nr = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+
+               for (i = 0; i < nr; i++)
+                       persistent_ram_free(cxt->fprzs[i]);
+               kfree(cxt->fprzs);
+       }
 }
 
 static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,

@@ -618,6 +654,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;

@@ -633,8 +670,9 @@ 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);
+       err = ramoops_init_przs(dev, cxt, &cxt->fprzs, &paddr, cxt->ftrace_size,
+                               (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1,
+                               LINUX_VERSION_CODE);
        if (err)
                goto fail_init_fprz;
 

@@ -698,7 +736,6 @@ fail_clear:
        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:

@@ -717,7 +754,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);
 

@@ -759,6 +795,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 = FTRACE_PER_CPU;
+
        /*
         * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
         * (using 1 byte for ECC isn't much of use anyway).

include/linux/pstore_ram.h +3 -0

index 69f3487..122f78d 100644
@@ -86,6 +86,8 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
  * @mem_address        physical memory address to contain ramoops
  */
 
+#define 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;
+       int             flags;
        struct persistent_ram_ecc_info ecc_info;
 };
 

Quoted 3 lines

--
2.7.4

Is there a case for not setting FTRACE_PER_CPU?

Quoted 198 lines · Kees Cook

Quoted 197 lines · Joel Fernandes

If FTRACE_PER_CPU flag is passed to ramoops pdata, split the 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]>

---
 fs/pstore/ram.c            | 72 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 58 insertions(+), 17 deletions(-)

fs/pstore/ram.c +55 -17

index 5caa512..5e96f78 100644
@@ -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,6 +97,7 @@ struct ramoops_context {
        size_t ftrace_size;
        size_t pmsg_size;
        int dump_oops;
+       int flags;
        struct persistent_ram_ecc_info ecc_info;
        unsigned int max_dump_cnt;
        unsigned int dump_write_cnt;

@@ -219,9 +220,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)) {
+               int max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               while (cxt->ftrace_read_cnt < max && !prz) {
+                       prz = ramoops_get_next_prz(cxt->fprzs,
+                                       &cxt->ftrace_read_cnt, max, 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 +292,23 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
                persistent_ram_write(cxt->cprz, buf, size, PSTORE_RAM_LOCK);
                return 0;
        } else if (type == PSTORE_TYPE_FTRACE) {
-               if (!cxt->fprz)
+               int zonenum, lock;
+
+               if (!cxt->fprzs)
                        return -ENOMEM;
-               persistent_ram_write(cxt->fprz, buf, size, PSTORE_RAM_LOCK);
+               /*
+                * If per-cpu buffers, don't lock. Otherwise there's only
+                * 1 zone for ftrace (zone 0) and all CPUs share it, so lock.
+                */
+               if (cxt->flags & FTRACE_PER_CPU) {
+                       zonenum = smp_processor_id();
+                       lock = PSTORE_RAM_NOLOCK;
+               } else {
+                       zonenum = 0;
+                       lock = PSTORE_RAM_LOCK;
+               }
+
+               persistent_ram_write(cxt->fprzs[zonenum], buf, size, lock);
                return 0;
        } else if (type == PSTORE_TYPE_PMSG) {
                pr_warn_ratelimited("ramoops: warning: PMSG shouldn't call %s\n",

@@ -352,6 +375,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
 {
        struct ramoops_context *cxt = psi->data;
        struct persistent_ram_zone *prz;
+       int max;
 
        switch (type) {
        case PSTORE_TYPE_DMESG:

@@ -363,7 +387,10 @@ 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;
+               max = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+               if (id >= max)
+                       return -EINVAL;
+               prz = cxt->fprzs[id];
                break;
        case PSTORE_TYPE_PMSG:
                prz = cxt->mprz;

@@ -394,14 +421,23 @@ 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) {
+               int nr = (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1;
+
+               for (i = 0; i < nr; i++)
+                       persistent_ram_free(cxt->fprzs[i]);
+               kfree(cxt->fprzs);
+       }
 }
 
 static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,

@@ -618,6 +654,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;

@@ -633,8 +670,9 @@ 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);
+       err = ramoops_init_przs(dev, cxt, &cxt->fprzs, &paddr, cxt->ftrace_size,
+                               (cxt->flags & FTRACE_PER_CPU) ? nr_cpu_ids : 1,
+                               LINUX_VERSION_CODE);
        if (err)
                goto fail_init_fprz;
 

@@ -698,7 +736,6 @@ fail_clear:
        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:

@@ -717,7 +754,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);
 

@@ -759,6 +795,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 = FTRACE_PER_CPU;
+
        /*
         * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
         * (using 1 byte for ECC isn't much of use anyway).

include/linux/pstore_ram.h +3 -0

index 69f3487..122f78d 100644
@@ -86,6 +86,8 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
  * @mem_address        physical memory address to contain ramoops
  */
 
+#define 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;
+       int             flags;
        struct persistent_ram_ecc_info ecc_info;
 };
 

Quoted 5 lines

Quoted 3 lines

--
2.7.4

Is there a case for not setting FTRACE_PER_CPU?

Yes, if there are too many CPUs and only a few are in use, then the rest of the space allocated for the other CPUs not in use go waste. If the ram zone is begin enough though, then it still makes sense even for the many CPUs case.

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

---
 fs/pstore/ftrace.c     |  3 +++
 fs/pstore/inode.c      |  7 ++---
 fs/pstore/internal.h   | 34 -------------------------
 include/linux/pstore.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 37 deletions(-)

fs/pstore/ftrace.c +0 -0

index d488770..1c6cf86 100644

fs/pstore/ftrace.c +2 -0

+
 static void notrace pstore_ftrace_call(unsigned long ip,
                                       unsigned long parent_ip,
                                       struct ftrace_ops *op,

@@ -42,6 +44,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);

fs/pstore/inode.c +4 -3

index ec9ddef..fcc5d1a 100644
@@ -107,9 +107,10 @@ 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;
 }

fs/pstore/internal.h +0 -34

index e38a22b..da416e6 100644
@@ -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);

include/linux/pstore.h +69 -1

index 92013cc..4e7fc3c 100644
@@ -89,4 +89,73 @@ 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
+ */
+
+#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
+ * else store it in the time stamp.
+ */
+#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

Quoted 28 lines · Joel Fernandes

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

---
 fs/pstore/ftrace.c     |  3 +++
 fs/pstore/inode.c      |  7 ++---
 fs/pstore/internal.h   | 34 -------------------------
 include/linux/pstore.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 37 deletions(-)

fs/pstore/ftrace.c +0 -0

index d488770..1c6cf86 100644
@@ -27,6 +27,8 @@
 #include <asm/barrier.h>
 #include "internal.h"
 

fs/pstore/ftrace.c line 30

Quoted 1 line

fs/pstore/ftrace.c +1 -0

+static u64 pstore_ftrace_stamp;

I think the race described in the commit message deserves a comment here.

Quoted 154 lines

fs/pstore/ftrace.c +2 -0

+
 static void notrace pstore_ftrace_call(unsigned long ip,
                                       unsigned long parent_ip,
                                       struct ftrace_ops *op,

@@ -42,6 +44,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);

fs/pstore/inode.c +4 -3

index ec9ddef..fcc5d1a 100644
@@ -107,9 +107,10 @@ 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;
 }

fs/pstore/internal.h +0 -34

index e38a22b..da416e6 100644
@@ -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);

include/linux/pstore.h +69 -1

index 92013cc..4e7fc3c 100644
@@ -89,4 +89,73 @@ 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
+ */
+
+#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
+ * else store it in the time stamp.
+ */
+#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*/
--

Quoted 2 lines

2.7.4

Should these ftrace helpers live somewhere else? It seems really specific to ftrace, not pstore?

-Kees

Quoted 29 lines · Kees Cook

Quoted 28 lines · Joel Fernandes

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

---
 fs/pstore/ftrace.c     |  3 +++
 fs/pstore/inode.c      |  7 ++---
 fs/pstore/internal.h   | 34 -------------------------
 include/linux/pstore.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 37 deletions(-)

fs/pstore/ftrace.c +0 -0

index d488770..1c6cf86 100644
@@ -27,6 +27,8 @@
 #include <asm/barrier.h>
 #include "internal.h"
 

Quoted 3 lines

fs/pstore/ftrace.c line 30

Quoted 1 line

fs/pstore/ftrace.c +1 -0

+static u64 pstore_ftrace_stamp;

I think the race described in the commit message deserves a comment here.

Sure, I can add that.

Quoted 160 lines

Quoted 154 lines

fs/pstore/ftrace.c +2 -0

+
 static void notrace pstore_ftrace_call(unsigned long ip,
                                       unsigned long parent_ip,
                                       struct ftrace_ops *op,

@@ -42,6 +44,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);

fs/pstore/inode.c +4 -3

index ec9ddef..fcc5d1a 100644
@@ -107,9 +107,10 @@ 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;
 }

fs/pstore/internal.h +0 -34

index e38a22b..da416e6 100644
@@ -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);

include/linux/pstore.h +69 -1

index 92013cc..4e7fc3c 100644
@@ -89,4 +89,73 @@ 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
+ */
+
+#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
+ * else store it in the time stamp.
+ */
+#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*/
--

Quoted 2 lines

2.7.4

Should these ftrace helpers live somewhere else? It seems really specific to ftrace, not pstore?

Quoted 2 lines

-Kees

Actually the time stamp and CPU information is not specific to low level ftrace hooks. Ftrace just provides the hooks into the function calls, its upto the user to store this information how they want. So the pstore ftrace helpers should be included in the pstore header IMO.