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, make the lock per-zone to protect the respective zone.

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

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, make the lock per-zone to protect the respective zone.

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

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 |  2 +-
 3 files changed, 22 insertions(+), 17 deletions(-)

fs/pstore/ram.c +5 -5

index f1219af..751197d 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, 1);
        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, 1);
                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, 1);
                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, 1);
                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, 1);
 
        cxt->dump_write_cnt = (cxt->dump_write_cnt + 1) % cxt->max_dump_cnt;
 

fs/pstore/ram_core.c +16 -11

index cb92055..c4722f0 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)
+               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)
+               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)
+               raw_spin_lock_irqsave(&prz->buffer_lock, flags);
 
        old = atomic_read(&prz->buffer->size);
        if (old == prz->buffer_size)

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

@@ -300,7 +305,7 @@ void persistent_ram_save_old(struct persistent_ram_zone *prz)
 }
 
 int notrace persistent_ram_write(struct persistent_ram_zone *prz,
-       const void *s, unsigned int count)
+       const void *s, unsigned int count, int lock)
 {
        int rem;
        int c = count;

@@ -311,9 +316,9 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
                c = prz->buffer_size;
        }
 
-       buffer_size_add(prz, c);
+       buffer_size_add(prz, c, lock);
 
-       start = buffer_start_add(prz, c);
+       start = buffer_start_add(prz, c, lock);
 
        rem = prz->buffer_size - start;
        if (unlikely(rem < c)) {

@@ -342,9 +347,9 @@ int notrace persistent_ram_write_user(struct persistent_ram_zone *prz,
                c = prz->buffer_size;
        }
 
-       buffer_size_add(prz, c);
+       buffer_size_add(prz, c, 1);
 
-       start = buffer_start_add(prz, c);
+       start = buffer_start_add(prz, c, 1);
 
        rem = prz->buffer_size - start;
        if (unlikely(rem < c)) {

include/linux/pstore_ram.h +1 -2

index 244d242..782af68 100644
@@ -61,7 +61,7 @@ void persistent_ram_free(struct persistent_ram_zone *prz);
 void persistent_ram_zap(struct persistent_ram_zone *prz);
 
 int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
-                        unsigned int count);
+                        unsigned int count, int lock);
 int persistent_ram_write_user(struct persistent_ram_zone *prz,
                              const void __user *s, unsigned int count);
 
--

2.7.4

Quoted 2 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.

This should be a bool argument (or enum), with named values so it's more readable. For example, these don't immediately tell me what the argument does:

persistent_ram_write(cxt->cprz, buf, size, 1);
persistent_ram_write(cxt->cprz, buf, size, true);

But this does:

persistent_ram_write(cxt->cprz, buf, size, PSTORE_REQUIRE_LOCKING);

As part of this, can you document in the pstore structure which types of front-ends require locking and if they don't, why?

PMSG now uses ramoops_pstore_write_buf_user instead of ramoops_pstore_write_buf Remove the case where we check PSTORE_TYPE_PMSG case.

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

---
 fs/pstore/ram.c | 5 -----
 1 file changed, 5 deletions(-)

fs/pstore/ram.c +0 -6

index 751197d..519404c 100644
@@ -287,11 +287,6 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
                        return -ENOMEM;
                persistent_ram_write(cxt->fprz, buf, size, 1);
                return 0;
-       } else if (type == PSTORE_TYPE_PMSG) {
-               if (!cxt->mprz)
-                       return -ENOMEM;
-               persistent_ram_write(cxt->mprz, buf, size, 1);
-               return 0;
        }
 
        if (type != PSTORE_TYPE_DMESG)
--

2.7.4

Quoted 2 lines · Joel Fernandes

PMSG now uses ramoops_pstore_write_buf_user instead of ramoops_pstore_write_buf Remove the case where we check PSTORE_TYPE_PMSG case.

Ah yeah, good point. Can you actually improve this to add a ratelimited WARN() to both _write_buf and write_buf_user when an unhandled type is encountered?

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 +9 -8

index 519404c..a796f49 100644
@@ -402,54 +402,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)

fs/pstore/ram.c +34 -14

+       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,

@@ -601,7 +621,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            | 70 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 56 insertions(+), 17 deletions(-)

fs/pstore/ram.c +0 -0

fs/pstore/ram.c +4 -4

@@ -391,14 +418,21 @@ 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]);

Quoted 14 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            | 70 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 56 insertions(+), 17 deletions(-)

fs/pstore/ram.c +0 -0

[..]

Quoted 17 lines

fs/pstore/ram.c +7 -4

@@ -391,14 +418,21 @@ 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;
+       }
 

fs/pstore/ram.c static void ramoops_free_przs(struct ramoops_context *cxt) · line 433

Quoted 6 lines

fs/pstore/ram.c +4 -2

-       kfree(cxt->przs);
-       cxt->max_dump_cnt = 0;
+       /* Free ftrace PRZs */
+       if (cxt->fprzs) {
+               for (i = 0; i < nr_cpu_ids; i++)
+                       persistent_ram_free(cxt->przs[i]);

I am supposed to free fprzs[i] here, instead of przs[i]. Also need to check flag and free correct number of zones. Will fix for v2, sorry about that.

Quoted 33 lines · Joel Fernandes

Quoted 14 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            | 70 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 56 insertions(+), 17 deletions(-)

fs/pstore/ram.c +0 -0

[..]

Quoted 17 lines

fs/pstore/ram.c +7 -4

@@ -391,14 +418,21 @@ 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;
+       }
 

Quoted 10 lines

fs/pstore/ram.c static void ramoops_free_przs(struct ramoops_context *cxt) · line 433

Quoted 6 lines

fs/pstore/ram.c +4 -2

-       kfree(cxt->przs);
-       cxt->max_dump_cnt = 0;
+       /* Free ftrace PRZs */
+       if (cxt->fprzs) {
+               for (i = 0; i < nr_cpu_ids; i++)
+                       persistent_ram_free(cxt->przs[i]);

I am supposed to free fprzs[i] here, instead of przs[i]. Also need to check flag and free correct number of zones. Will fix for v2, sorry about that.

I think the cpu id needs to be bounds-checked against the size of the allocation. In theory, CPU hot-plug could grow the number of CPUs after pstore is initialized.

Quoted 33 lines · Joel Fernandes

Quoted 14 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            | 70 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 56 insertions(+), 17 deletions(-)

fs/pstore/ram.c +0 -0

[..]

Quoted 17 lines

fs/pstore/ram.c +7 -4

@@ -391,14 +418,21 @@ 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;
+       }
 

Quoted 10 lines

fs/pstore/ram.c static void ramoops_free_przs(struct ramoops_context *cxt) · line 433

Quoted 6 lines

fs/pstore/ram.c +4 -2

-       kfree(cxt->przs);
-       cxt->max_dump_cnt = 0;
+       /* Free ftrace PRZs */
+       if (cxt->fprzs) {
+               for (i = 0; i < nr_cpu_ids; i++)
+                       persistent_ram_free(cxt->przs[i]);

I am supposed to free fprzs[i] here, instead of przs[i]. Also need to check flag and free correct number of zones. Will fix for v2, sorry about that.

I think the cpu id needs to be bounds-checked against the size of the allocation. In theory, CPU hot-plug could grow the number of CPUs after pstore is initialized.

Hi Kees,

Quoted 45 lines · Kees Cook

Quoted 43 lines · Joel Fernandes

Quoted 14 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            | 70 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pstore_ram.h |  3 ++
 2 files changed, 56 insertions(+), 17 deletions(-)

fs/pstore/ram.c +0 -0

[..]

Quoted 13 lines

fs/pstore/ram.c +4 -4

@@ -391,14 +418,21 @@ 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]);

fs/pstore/ram.c static void ramoops_free_przs(struct ramoops_context *cxt) · line 433

Quoted 10 lines

fs/pstore/ram.c +7 -2

+               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 < nr_cpu_ids; i++)
+                       persistent_ram_free(cxt->przs[i]);

I am supposed to free fprzs[i] here, instead of przs[i]. Also need to check flag and free correct number of zones. Will fix for v2, sorry about that.

Quoted 3 lines

I think the cpu id needs to be bounds-checked against the size of the allocation. In theory, CPU hot-plug could grow the number of CPUs after pstore is initialized.

nr_cpu_ids is fixed to the number of possible CPUs regardless of if hotplug is being used or not. I did a hotplug test as well to confirm this. So if I boot on 4 CPU machine with only 2 CPUs online, then nr_cpu_ids is 4.

Here's an early RFC for a patch series on improving ftrace throughput with ramoops. I am hoping to get some early comments so I'm releasing it in advance. It is functional and tested.

Currently ramoops uses a single zone to store function traces. To make this work, it has to uses locking to synchronize accesses to the buffers. Recently the synchronization was completely moved from a cmpxchg mechanism to raw spinlocks due to difficulties in using cmpxchg on uncached memory and also on RAMs behind PCIe. [1] This change further dropped the peformance of ramoops pstore backend by more than half in my tests.

This patch series improves the situation dramatically by around 280% from what it is now by creating a ramoops persistent zone for each CPU and avoiding use of locking altogether for ftrace. At init time, the persistent zones are then merged together.

Here are some tests to show the improvements. Tested using a qemu quad core x86_64 instance with -mem-path to persist the guest RAM to a file. I measured avergage throughput of dd over 30 seconds:

dd if=/dev/zero | pv | dd of=/dev/null

Without this patch series: 24MB/s
With per-cpu buffers and counter increment: 91.5 MB/s (improvement by ~ 281%) with per-cpu buffers and trace_clock: 51.9 MB/s

Some more considerations:
1. Inorder to do the merge of the individual buffers, I am using racy counters since I didn't want to sacrifice throughput for perfect time stamps. trace_clock() for timestamps although did the job but was almost half the throughput of using counter based timestamp.

2. Since the patches divide the available ftrace persistent space by the number of CPUs, lesser space will now be available per-CPU however the user is free to disable per CPU behavior and revert to the old behavior by specifying PSTORE_PER_CPU flag. Its a space vs performance trade-off so if user has enough space and not a lot of CPUs, then using per-CPU persistent buffers make sense for better performance.

3. Without using any counters or timestamps, the improvement is even more (~140MB/s) but the buffers cannot be merged.

[1] https://lkml.org/lkml/2016/9/8/375

From a tracing point of view, I have no qualms with this patch set.

Joel Fernandes (7):
pstore: Make spinlock per zone instead of global
pstore: locking: dont lock unless caller asks to
pstore: Remove case of PSTORE_TYPE_PMSG write using deprecated
function
pstore: Make ramoops_init_przs generic for other prz arrays
ramoops: Split ftrace buffer space into per-CPU zones
pstore: Add support to store timestamp counter in ftrace records
pstore: Merge per-CPU ftrace zones into one zone for output

 fs/pstore/ftrace.c         |   3 +
 fs/pstore/inode.c          |   7 +-
 fs/pstore/internal.h       |  34 -------
 fs/pstore/ram.c            | 234 +++++++++++++++++++++++++++++++++++----------
 fs/pstore/ram_core.c       |  30 +++---
 include/linux/pstore.h     |  69 +++++++++++++
 include/linux/pstore_ram.h |   6 +-
 7 files changed, 280 insertions(+), 103 deletions(-)