* [PATCH] pstore: drop pmsg bounce buffer
@ 2016-09-01 15:13 Mark Salyzyn
2016-09-08 20:04 ` Kees Cook
0 siblings, 1 reply; 2+ messages in thread
From: Mark Salyzyn @ 2016-09-01 15:13 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Salyzyn, Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Removing a bounce buffer copy operation in the pmsg driver path is
always better. We also gain in overall performance by not requesting
a vmalloc on every write as this can cause precious RT tasks, such
as user facing media operation, to stall while memory is being
reclaimed. Added a write_buf_user to the pstore functions, a backup
platform write_buf_user that uses the small buffer that is part of
the instance, and implemented a ramoops write_buf_user that only
supports PSTORE_TYPE_PMSG.
Signed-off-by: Mark Salyzyn <salyzyn@android.com>
---
fs/pstore/platform.c | 36 +++++++++++++++++++++++++++++++++++
fs/pstore/pmsg.c | 35 ++++++----------------------------
fs/pstore/ram.c | 19 +++++++++++++++++++
fs/pstore/ram_core.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
include/linux/pstore.h | 11 ++++++++---
include/linux/pstore_ram.h | 7 +++++--
6 files changed, 119 insertions(+), 36 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 16ecca5..b9840a6 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -623,6 +623,40 @@ static int pstore_write_compat(enum pstore_type_id type,
size, psi);
}
+static int pstore_write_buf_user_compat(enum pstore_type_id type,
+ enum kmsg_dump_reason reason,
+ u64 *id, unsigned int part,
+ const char __user *buf,
+ bool compressed, size_t size,
+ struct pstore_info *psi)
+{
+ unsigned long flags = 0;
+ size_t i, bufsize = size;
+ long ret = 0;
+
+ if (unlikely(!access_ok(VERIFY_READ, buf, size)))
+ return -EFAULT;
+ if (bufsize > psinfo->bufsize)
+ bufsize = psinfo->bufsize;
+ spin_lock_irqsave(&psinfo->buf_lock, flags);
+ for (i = 0; i < size; ) {
+ size_t c = min(size - i, bufsize);
+
+ ret = __copy_from_user(psinfo->buf, buf + i, c);
+ if (unlikely(ret != 0)) {
+ ret = -EFAULT;
+ break;
+ }
+ ret = psi->write_buf(type, reason, id, part, psinfo->buf,
+ compressed, c, psi);
+ if (unlikely(ret < 0))
+ break;
+ i += c;
+ }
+ spin_unlock_irqrestore(&psinfo->buf_lock, flags);
+ return unlikely(ret < 0) ? ret : size;
+}
+
/*
* platform specific persistent storage driver registers with
* us here. If pstore is already mounted, call the platform
@@ -645,6 +679,8 @@ int pstore_register(struct pstore_info *psi)
if (!psi->write)
psi->write = pstore_write_compat;
+ if (!psi->write_buf_user)
+ psi->write_buf_user = pstore_write_buf_user_compat;
psinfo = psi;
mutex_init(&psinfo->read_mutex);
spin_unlock(&pstore_lock);
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 7de20cd..78f6176 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -19,48 +19,25 @@
#include "internal.h"
static DEFINE_MUTEX(pmsg_lock);
-#define PMSG_MAX_BOUNCE_BUFFER_SIZE (2*PAGE_SIZE)
static ssize_t write_pmsg(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- size_t i, buffer_size;
- char *buffer;
+ u64 id;
+ int ret;
if (!count)
return 0;
+ /* check outside lock, page in any data. write_buf_user also checks */
if (!access_ok(VERIFY_READ, buf, count))
return -EFAULT;
- buffer_size = count;
- if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
- buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
- buffer = vmalloc(buffer_size);
- if (!buffer)
- return -ENOMEM;
-
mutex_lock(&pmsg_lock);
- for (i = 0; i < count; ) {
- size_t c = min(count - i, buffer_size);
- u64 id;
- long ret;
-
- ret = __copy_from_user(buffer, buf + i, c);
- if (unlikely(ret != 0)) {
- mutex_unlock(&pmsg_lock);
- vfree(buffer);
- return -EFAULT;
- }
- psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
- psinfo);
-
- i += c;
- }
-
+ ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
+ psinfo);
mutex_unlock(&pmsg_lock);
- vfree(buffer);
- return count;
+ return ret ? ret : count;
}
static const struct file_operations pmsg_fops = {
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 7a034d6..893e040c 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -331,6 +331,24 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
return 0;
}
+static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
+ enum kmsg_dump_reason reason,
+ u64 *id, unsigned int part,
+ const char __user *buf,
+ bool compressed, size_t size,
+ struct pstore_info *psi)
+{
+ if (type == PSTORE_TYPE_PMSG) {
+ struct ramoops_context *cxt = psi->data;
+
+ if (!cxt->mprz)
+ return -ENOMEM;
+ return persistent_ram_write_user(cxt->mprz, buf, size);
+ }
+
+ return -EINVAL;
+}
+
static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
struct timespec time, struct pstore_info *psi)
{
@@ -369,6 +387,7 @@ static struct ramoops_context oops_cxt = {
.open = ramoops_pstore_open,
.read = ramoops_pstore_read,
.write_buf = ramoops_pstore_write_buf,
+ .write_buf_user = ramoops_pstore_write_buf_user,
.erase = ramoops_pstore_erase,
},
};
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 76c3f80..aa9afe5 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -17,15 +17,16 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/errno.h>
-#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/io.h>
+#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/memblock.h>
+#include <linux/pstore_ram.h>
#include <linux/rslib.h>
#include <linux/slab.h>
+#include <linux/uaccess.h>
#include <linux/vmalloc.h>
-#include <linux/pstore_ram.h>
#include <asm/page.h>
struct persistent_ram_buffer {
@@ -303,6 +304,16 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz,
persistent_ram_update_ecc(prz, start, count);
}
+static int notrace persistent_ram_update_user(struct persistent_ram_zone *prz,
+ const void __user *s, unsigned int start, unsigned int count)
+{
+ struct persistent_ram_buffer *buffer = prz->buffer;
+ int ret = unlikely(__copy_from_user(buffer->data + start, s, count)) ?
+ -EFAULT : 0;
+ persistent_ram_update_ecc(prz, start, count);
+ return ret;
+}
+
void persistent_ram_save_old(struct persistent_ram_zone *prz)
{
struct persistent_ram_buffer *buffer = prz->buffer;
@@ -356,6 +367,38 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
return count;
}
+int notrace persistent_ram_write_user(struct persistent_ram_zone *prz,
+ const void __user *s, unsigned int count)
+{
+ int rem, ret = 0, c = count;
+ size_t start;
+
+ if (unlikely(!access_ok(VERIFY_READ, s, count)))
+ return -EFAULT;
+ if (unlikely(c > prz->buffer_size)) {
+ s += c - prz->buffer_size;
+ c = prz->buffer_size;
+ }
+
+ buffer_size_add(prz, c);
+
+ start = buffer_start_add(prz, c);
+
+ rem = prz->buffer_size - start;
+ if (unlikely(rem < c)) {
+ ret = persistent_ram_update_user(prz, s, start, rem);
+ s += rem;
+ c -= rem;
+ start = 0;
+ }
+ if (likely(!ret))
+ ret = persistent_ram_update_user(prz, s, start, c);
+
+ persistent_ram_update_header_ecc(prz);
+
+ return unlikely(ret) ? ret : count;
+}
+
size_t persistent_ram_old_size(struct persistent_ram_zone *prz)
{
return prz->old_log_size;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 899e95e..2349d7d 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -22,12 +22,13 @@
#ifndef _LINUX_PSTORE_H
#define _LINUX_PSTORE_H
-#include <linux/time.h>
+#include <linux/compiler.h>
+#include <linux/errno.h>
#include <linux/kmsg_dump.h>
#include <linux/mutex.h>
-#include <linux/types.h>
#include <linux/spinlock.h>
-#include <linux/errno.h>
+#include <linux/time.h>
+#include <linux/types.h>
/* types */
enum pstore_type_id {
@@ -68,6 +69,10 @@ struct pstore_info {
enum kmsg_dump_reason reason, u64 *id,
unsigned int part, const char *buf, bool compressed,
size_t size, struct pstore_info *psi);
+ int (*write_buf_user)(enum pstore_type_id type,
+ enum kmsg_dump_reason reason, u64 *id,
+ unsigned int part, const char __user *buf,
+ bool compressed, size_t size, struct pstore_info *psi);
int (*erase)(enum pstore_type_id type, u64 id,
int count, struct timespec time,
struct pstore_info *psi);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 4660aaa..c668c86 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -17,11 +17,12 @@
#ifndef __LINUX_PSTORE_RAM_H__
#define __LINUX_PSTORE_RAM_H__
+#include <linux/compiler.h>
#include <linux/device.h>
+#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/types.h>
-#include <linux/init.h>
struct persistent_ram_buffer;
struct rs_control;
@@ -59,7 +60,9 @@ 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 persistent_ram_write_user(struct persistent_ram_zone *prz,
+ const void __user *s, unsigned int count);
void persistent_ram_save_old(struct persistent_ram_zone *prz);
size_t persistent_ram_old_size(struct persistent_ram_zone *prz);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] pstore: drop pmsg bounce buffer
2016-09-01 15:13 [PATCH] pstore: drop pmsg bounce buffer Mark Salyzyn
@ 2016-09-08 20:04 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2016-09-08 20:04 UTC (permalink / raw)
To: Mark Salyzyn; +Cc: LKML, Anton Vorontsov, Colin Cross, Tony Luck
On Thu, Sep 1, 2016 at 8:13 AM, Mark Salyzyn <salyzyn@android.com> wrote:
> Removing a bounce buffer copy operation in the pmsg driver path is
> always better. We also gain in overall performance by not requesting
> a vmalloc on every write as this can cause precious RT tasks, such
> as user facing media operation, to stall while memory is being
> reclaimed. Added a write_buf_user to the pstore functions, a backup
> platform write_buf_user that uses the small buffer that is part of
> the instance, and implemented a ramoops write_buf_user that only
> supports PSTORE_TYPE_PMSG.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
I've applied this, thanks!
> ---
> fs/pstore/platform.c | 36 +++++++++++++++++++++++++++++++++++
> fs/pstore/pmsg.c | 35 ++++++----------------------------
> fs/pstore/ram.c | 19 +++++++++++++++++++
> fs/pstore/ram_core.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/pstore.h | 11 ++++++++---
> include/linux/pstore_ram.h | 7 +++++--
> 6 files changed, 119 insertions(+), 36 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 16ecca5..b9840a6 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -623,6 +623,40 @@ static int pstore_write_compat(enum pstore_type_id type,
> size, psi);
> }
>
> +static int pstore_write_buf_user_compat(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id, unsigned int part,
> + const char __user *buf,
> + bool compressed, size_t size,
> + struct pstore_info *psi)
> +{
> + unsigned long flags = 0;
> + size_t i, bufsize = size;
> + long ret = 0;
> +
> + if (unlikely(!access_ok(VERIFY_READ, buf, size)))
> + return -EFAULT;
> + if (bufsize > psinfo->bufsize)
> + bufsize = psinfo->bufsize;
> + spin_lock_irqsave(&psinfo->buf_lock, flags);
> + for (i = 0; i < size; ) {
> + size_t c = min(size - i, bufsize);
> +
> + ret = __copy_from_user(psinfo->buf, buf + i, c);
> + if (unlikely(ret != 0)) {
> + ret = -EFAULT;
> + break;
> + }
> + ret = psi->write_buf(type, reason, id, part, psinfo->buf,
> + compressed, c, psi);
> + if (unlikely(ret < 0))
> + break;
> + i += c;
> + }
> + spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> + return unlikely(ret < 0) ? ret : size;
> +}
> +
> /*
> * platform specific persistent storage driver registers with
> * us here. If pstore is already mounted, call the platform
> @@ -645,6 +679,8 @@ int pstore_register(struct pstore_info *psi)
>
> if (!psi->write)
> psi->write = pstore_write_compat;
> + if (!psi->write_buf_user)
> + psi->write_buf_user = pstore_write_buf_user_compat;
> psinfo = psi;
> mutex_init(&psinfo->read_mutex);
> spin_unlock(&pstore_lock);
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 7de20cd..78f6176 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -19,48 +19,25 @@
> #include "internal.h"
>
> static DEFINE_MUTEX(pmsg_lock);
> -#define PMSG_MAX_BOUNCE_BUFFER_SIZE (2*PAGE_SIZE)
>
> static ssize_t write_pmsg(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> - size_t i, buffer_size;
> - char *buffer;
> + u64 id;
> + int ret;
>
> if (!count)
> return 0;
>
> + /* check outside lock, page in any data. write_buf_user also checks */
> if (!access_ok(VERIFY_READ, buf, count))
> return -EFAULT;
>
> - buffer_size = count;
> - if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
> - buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
> - buffer = vmalloc(buffer_size);
> - if (!buffer)
> - return -ENOMEM;
> -
> mutex_lock(&pmsg_lock);
> - for (i = 0; i < count; ) {
> - size_t c = min(count - i, buffer_size);
> - u64 id;
> - long ret;
> -
> - ret = __copy_from_user(buffer, buf + i, c);
> - if (unlikely(ret != 0)) {
> - mutex_unlock(&pmsg_lock);
> - vfree(buffer);
> - return -EFAULT;
> - }
> - psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
> - psinfo);
> -
> - i += c;
> - }
> -
> + ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
> + psinfo);
> mutex_unlock(&pmsg_lock);
> - vfree(buffer);
> - return count;
> + return ret ? ret : count;
> }
>
> static const struct file_operations pmsg_fops = {
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 7a034d6..893e040c 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -331,6 +331,24 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
> return 0;
> }
>
> +static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id, unsigned int part,
> + const char __user *buf,
> + bool compressed, size_t size,
> + struct pstore_info *psi)
> +{
> + if (type == PSTORE_TYPE_PMSG) {
> + struct ramoops_context *cxt = psi->data;
> +
> + if (!cxt->mprz)
> + return -ENOMEM;
> + return persistent_ram_write_user(cxt->mprz, buf, size);
> + }
> +
> + return -EINVAL;
> +}
> +
> static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
> struct timespec time, struct pstore_info *psi)
> {
> @@ -369,6 +387,7 @@ static struct ramoops_context oops_cxt = {
> .open = ramoops_pstore_open,
> .read = ramoops_pstore_read,
> .write_buf = ramoops_pstore_write_buf,
> + .write_buf_user = ramoops_pstore_write_buf_user,
> .erase = ramoops_pstore_erase,
> },
> };
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 76c3f80..aa9afe5 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -17,15 +17,16 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> -#include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/io.h>
> +#include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/memblock.h>
> +#include <linux/pstore_ram.h>
> #include <linux/rslib.h>
> #include <linux/slab.h>
> +#include <linux/uaccess.h>
> #include <linux/vmalloc.h>
> -#include <linux/pstore_ram.h>
> #include <asm/page.h>
>
> struct persistent_ram_buffer {
> @@ -303,6 +304,16 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz,
> persistent_ram_update_ecc(prz, start, count);
> }
>
> +static int notrace persistent_ram_update_user(struct persistent_ram_zone *prz,
> + const void __user *s, unsigned int start, unsigned int count)
> +{
> + struct persistent_ram_buffer *buffer = prz->buffer;
> + int ret = unlikely(__copy_from_user(buffer->data + start, s, count)) ?
> + -EFAULT : 0;
> + persistent_ram_update_ecc(prz, start, count);
> + return ret;
> +}
> +
> void persistent_ram_save_old(struct persistent_ram_zone *prz)
> {
> struct persistent_ram_buffer *buffer = prz->buffer;
> @@ -356,6 +367,38 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
> return count;
> }
>
> +int notrace persistent_ram_write_user(struct persistent_ram_zone *prz,
> + const void __user *s, unsigned int count)
> +{
> + int rem, ret = 0, c = count;
> + size_t start;
> +
> + if (unlikely(!access_ok(VERIFY_READ, s, count)))
> + return -EFAULT;
> + if (unlikely(c > prz->buffer_size)) {
> + s += c - prz->buffer_size;
> + c = prz->buffer_size;
> + }
> +
> + buffer_size_add(prz, c);
> +
> + start = buffer_start_add(prz, c);
> +
> + rem = prz->buffer_size - start;
> + if (unlikely(rem < c)) {
> + ret = persistent_ram_update_user(prz, s, start, rem);
> + s += rem;
> + c -= rem;
> + start = 0;
> + }
> + if (likely(!ret))
> + ret = persistent_ram_update_user(prz, s, start, c);
> +
> + persistent_ram_update_header_ecc(prz);
> +
> + return unlikely(ret) ? ret : count;
> +}
> +
As a follow-up, I wonder if ram_write and ram_write_user could be
consolidated in some way since they're identical except for the update
function, and contain a lot of non-obvious work that should stay the
same between both functions (and could probably use more comments).
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-09-08 20:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-01 15:13 [PATCH] pstore: drop pmsg bounce buffer Mark Salyzyn
2016-09-08 20:04 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).