Allow access to sysfs attributes without mem allocations.

PATCH Superseded NeilBrown 2 participants 6 messages Sep 30, 2014 – Oct 5, 2014

Subsystems (2)

Driver core kernfs

lore

Share & tools

lore

NeilBrown

author

2014-09-30 02:41 UTC #0 lore raw

cover Allow access to sysfs attributes without mem allocations.

Hi Tejun,
is this closer to what you would like?

I do really need this functionality, but I honestly don't like this patch.
The need to identify just which attributes need special care seems backwards to me.
1/ it is the process (which has called mlock etc) which needs
special care, more than the attribute. Everything that process does needs to avoid allocating memory, whether that this is particularly related to enabling write-out or not.
2/ It is also backwards because the vast majority of sysfs
attributes only support bool/enum/int which means at most 23 chars including sign and newline (maybe more for reads if the read provides a list of options). So almost everything doesn't need an allocation at all - just some on-stack space. I would be quite happy to identify those attributes that may need care when accessing and could possibly supports read or write > 128 characters, because there wouldn't be any.

I also don't like this approach because we end up allocating 2 pages for the buffer, as we have to ask for "PAGE_SIZE+1" bytes, for the hypothetical case that an important attribute actually requires a full PAGE_SIZE write...

Would you be happy to have all specially identified attributes be limited to 127 characters IO max? Then I would just use an on-stack buffer for those which would remove the allocation and simplify some of the code.

Thanks,
NeilBrown

---

NeilBrown (2):
sysfs - allow attributes to request write buffer be pre-allocated. sysfs/kernfs: make read requests on pre-alloc files use the buffer.

 fs/kernfs/file.c       |   52 +++++++++++++++++++++++++++++++----------------
 fs/sysfs/file.c        |   53 +++++++++++++++++++++++++++++++++++++++++-------
 include/linux/kernfs.h |    2 ++
 include/linux/sysfs.h  |    9 ++++++++
 4 files changed, 90 insertions(+), 26 deletions(-)

Signature · 1 line

Signature

NeilBrown

author

2014-09-30 02:42 UTC #1 lore raw

PATCH 1/2 sysfs - allow attributes to request write buffer be pre-allocated.

md/raid allows metadata management to be performed in user-space. A various times, particularly on device failure, the metadata needs to be updated before further writes can be permitted. This means that the user-space program which updates metadata much not block on writeout, and so must not allocate memory.

mlockall(MCL_CURRENT|MCL_FUTURE) and pre-allocation can avoid all memory allocation issues for user-memory, but that does not help kernel memory.
Several kernel objects can be pre-allocated. e.g. files opened before any writes to the array are permitted.
However some kernel allocation happen in places that cannot be pre-allocated.
In particular, writes to sysfs files (to tell md that it can now allow writes to the array) allocate a buffer using GFP_KERNEL.

This patch allows attributes to be marked as "PREALLOC". In that case the maximal buffer is allocated when the file is opened, and then used on each write instead of allocating a new buffer.

As the same buffer is now shared for all writes on the same file description, the mutex is extended to cover full use of the buffer including the copy_from_user().

The new __ATTR_PREALLOC() 'or's a new flag in to the 'mode', which is inspected by sysfs_add_file_mode_ns to determine if the file should be marked as requiring prealloc.

Signed-off-by NeilBrown <[email protected]>

---
 fs/kernfs/file.c       |   35 ++++++++++++++++++++++++-----------
 fs/sysfs/file.c        |   31 ++++++++++++++++++++++++-------
 include/linux/kernfs.h |    2 ++
 include/linux/sysfs.h  |    9 +++++++++
 4 files changed, 59 insertions(+), 18 deletions(-)

fs/kernfs/file.c +24 ~~-11~~

index 4429d6d9217f..73bd5ed143cd 100644
@@ -278,16 +278,12 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		len = min_t(size_t, count, PAGE_SIZE);
 	}
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	buf = of->buf;
+	if (!buf)
+		buf = kmalloc(len + 1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	if (copy_from_user(buf, user_buf, len)) {
-		len = -EFAULT;
-		goto out_free;
-	}
-	buf[len] = '\0';	/* guarantee string termination */
-
 	/*
 	 * @of->mutex nests outside active ref and is just to ensure that
 	 * the ops aren't called concurrently for the same open file.

@@ -299,19 +295,27 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		goto out_free;
 	}
 
+	if (copy_from_user(buf, user_buf, len)) {
+		len = -EFAULT;
+		goto out_unlock;
+	}
+	buf[len] = '\0';	/* guarantee string termination */
+
 	ops = kernfs_ops(of->kn);
 	if (ops->write)
 		len = ops->write(of, buf, len, *ppos);
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len > 0)
 		*ppos += len;
+
+out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
 out_free:
-	kfree(buf);
+	if (buf != of->buf)
+		kfree(buf);
 	return len;
 }

@@ -685,6 +689,13 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 	 */
 	of->atomic_write_len = ops->atomic_write_len;
 
+	if (ops->prealloc) {
+		int len = of->atomic_write_len ?: PAGE_SIZE;
+		of->buf = kmalloc(len + 1, GFP_KERNEL);
+		error = -ENOMEM;
+		if (!of->buf)
+			goto err_free;
+	}
 	/*
 	 * Always instantiate seq_file even if read access doesn't use
 	 * seq_file or is not requested.  This unifies private data access

@@ -715,6 +726,7 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 err_close:
 	seq_release(inode, file);
 err_free:
+	kfree(of->buf);
 	kfree(of);
 err_out:
 	kernfs_put_active(kn);

@@ -728,6 +740,7 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
 
 	kernfs_put_open_node(kn, of);
 	seq_release(inode, filp);
+	kfree(of->buf);
 	kfree(of);
 
 	return 0;

fs/sysfs/file.c +24 -7

index e9ef59b3abb1..4a959d231b43 100644
@@ -184,6 +184,17 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
+	.write		= sysfs_kf_write,
+	.prealloc	= true,
+};
+
+static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
+	.seq_show	= sysfs_kf_seq_show,
+	.write		= sysfs_kf_write,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_bin_kfops_ro = {
 	.read		= sysfs_kf_bin_read,
 };

@@ -222,13 +233,19 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 			 kobject_name(kobj)))
 			return -EINVAL;
 
-		if (sysfs_ops->show && sysfs_ops->store)
-			ops = &sysfs_file_kfops_rw;
-		else if (sysfs_ops->show)
+		if (sysfs_ops->show && sysfs_ops->store) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_rw;
+			else
+				ops = &sysfs_file_kfops_rw;
+		} else if (sysfs_ops->show)
 			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store)
-			ops = &sysfs_file_kfops_wo;
-		else
+		else if (sysfs_ops->store) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_wo;
+			else
+				ops = &sysfs_file_kfops_wo;
+		} else
 			ops = &sysfs_file_kfops_empty;
 
 		size = PAGE_SIZE;

@@ -253,7 +270,7 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 	if (!attr->ignore_lockdep)
 		key = attr->key ?: (struct lock_class_key *)&attr->skey;
 #endif
-	kn = __kernfs_create_file(parent, attr->name, mode, size, ops,
+	kn = __kernfs_create_file(parent, attr->name, mode & 0777, size, ops,
 				  (void *)attr, ns, true, key);
 	if (IS_ERR(kn)) {
 		if (PTR_ERR(kn) == -EEXIST)

include/linux/kernfs.h +2 -0

index 30faf797c2c3..07c326761671 100644
@@ -179,6 +179,7 @@ struct kernfs_open_file {
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
+	char			*buf;
 
 	size_t			atomic_write_len;
 	bool			mmapped;

@@ -214,6 +215,7 @@ struct kernfs_ops {
 	 * larger ones are rejected with -E2BIG.
 	 */
 	size_t atomic_write_len;
+	bool prealloc;
 	ssize_t (*write)(struct kernfs_open_file *of, char *buf, size_t bytes,
 			 loff_t off);

include/linux/sysfs.h +9 -0

index f97d0dbb59fa..ddad16148bd6 100644
@@ -70,6 +70,8 @@ struct attribute_group {
  * for examples..
  */
 
+#define SYSFS_PREALLOC 010000
+
 #define __ATTR(_name, _mode, _show, _store) {				\
 	.attr = {.name = __stringify(_name),				\
 		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\

@@ -77,6 +79,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
+#define __ATTR_PREALLOC(_name, _mode, _show, _store) {			\
+	.attr = {.name = __stringify(_name),				\
+		 .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
+	.show	= _show,						\
+	.store	= _store,						\
+}
+
 #define __ATTR_RO(_name) {						\
 	.attr	= { .name = __stringify(_name), .mode = S_IRUGO },	\
 	.show	= _name##_show,						\

Tejun Heo

maintainer

2014-10-05 19:56 UTC #2 lore raw

Re: [PATCH 1/2] sysfs - allow attributes to request write buffer be pre-allocated.

Hello,

Generally looks good to me. Some nitpicks follow.

On Tue, Sep 30, 2014 at 12:33:34PM +1000, NeilBrown wrote:
...

Quoted 14 lines

fs/kernfs/file.c +3 -1

index 4429d6d9217f..73bd5ed143cd 100644
@@ -278,16 +278,12 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		len = min_t(size_t, count, PAGE_SIZE);
 	}
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	buf = of->buf;
+	if (!buf)
+		buf = kmalloc(len + 1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;

fs/kernfs/file.c static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf, · line 289

Quoted 10 lines

fs/kernfs/file.c +0 -6

 
-	if (copy_from_user(buf, user_buf, len)) {
-		len = -EFAULT;
-		goto out_free;
-	}
-	buf[len] = '\0';	/* guarantee string termination */
-
 	/*
 	 * @of->mutex nests outside active ref and is just to ensure that
 	 * the ops aren't called concurrently for the same open file.

We probably should update the above comment.

include/linux/kernfs.h struct kernfs_open_file { · line 182

Quoted 9 lines

include/linux/kernfs.h +1 -0

index 30faf797c2c3..07c326761671 100644
@@ -179,6 +179,7 @@ struct kernfs_open_file {
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
+	char			*buf;

Maybe something like ->prealloc_buf is better?

Thanks.

Signature · 1 line

tejun

NeilBrown

author

2014-09-30 02:42 UTC #3 lore raw

PATCH 2/2 sysfs/kernfs: make read requests on pre-alloc files use the buffer.

In reply to NeilBrown

To match the previous patch which used the pre-alloc buffer for writes, this patch causes reads to use the same buffer. This is not strictly necessary as the current seq_read() will allocate on first read, so user-space can trigger the required pre-alloc. But consistency is valuable.

The read function is somewhat simpler than seq_read() and, for example, does not support reading from an offset into the file: reads must be at the start of the file.

As the buffer is shared with writes and other reads, the mutex is extended to cover the copy_to_user.

Signed-off-by NeilBrown <[email protected]>

---
 fs/kernfs/file.c |   17 ++++++++++-------
 fs/sysfs/file.c  |   28 ++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 11 deletions(-)

fs/kernfs/file.c +10 -7

index 73bd5ed143cd..7072240604f5 100644
@@ -189,7 +189,9 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;

@@ -210,21 +212,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;
 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->buf)
+		kfree(buf);
 	return len;
 }

fs/sysfs/file.c +24 -4

index 4a959d231b43..3f7e4d341546 100644
@@ -102,6 +102,18 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 	return battr->read(of->file, kobj, battr, buf, pos, count);
 }
 
+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+                            size_t count, loff_t pos)
+{
+       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+       struct kobject *kobj = of->kn->parent->priv;
+
+       if (pos)
+               return 0;
+       return ops->show(kobj, of->kn->priv, buf);
+}
+
 /* kernfs write callback for regular sysfs files */
 static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,
 			      size_t count, loff_t pos)

@@ -184,13 +196,18 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_ro = {
+	.read		= sysfs_kf_read,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
 
 static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
-	.seq_show	= sysfs_kf_seq_show,
+	.read		= sysfs_kf_read,
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };

@@ -238,9 +255,12 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 				ops = &sysfs_prealloc_kfops_rw;
 			else
 				ops = &sysfs_file_kfops_rw;
-		} else if (sysfs_ops->show)
-			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store) {
+		} else if (sysfs_ops->show) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_ro;
+			else
+				ops = &sysfs_file_kfops_ro;
+		} else if (sysfs_ops->store) {
 			if (mode & SYSFS_PREALLOC)
 				ops = &sysfs_prealloc_kfops_wo;
 			else

Tejun Heo

maintainer

2014-10-05 20:02 UTC #4 lore raw

Re: [PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

Quoted 52 lines · NeilBrown

To match the previous patch which used the pre-alloc buffer for writes, this patch causes reads to use the same buffer. This is not strictly necessary as the current seq_read() will allocate on first read, so user-space can trigger the required pre-alloc. But consistency is valuable.
The read function is somewhat simpler than seq_read() and, for example, does not support reading from an offset into the file: reads must be at the start of the file.
As the buffer is shared with writes and other reads, the mutex is extended to cover the copy_to_user.
Signed-off-by NeilBrown <[email protected]>
---
 fs/kernfs/file.c |   17 ++++++++++-------
 fs/sysfs/file.c  |   28 ++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 11 deletions(-)
fs/kernfs/file.c +5 -6
index 73bd5ed143cd..7072240604f5 100644
@@ -189,7 +189,9 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -210,21 +212,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;

fs/kernfs/file.c static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of, · line 232

Quoted 10 lines

fs/kernfs/file.c +5 -1

 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->buf)
+		kfree(buf);
 	return len;
 }

Can you please also make kernfs reject ->seq_show() if .prealloc is being used?

fs/kernfs/file.c static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,

Quoted 10 lines

fs/kernfs/file.c +10 -0

+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+                            size_t count, loff_t pos)
+{
+       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+       struct kobject *kobj = of->kn->parent->priv;
+
+       if (pos)
+               return 0;
+       return ops->show(kobj, of->kn->priv, buf);

Shouldn't it also memset?

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer

2014-10-05 19:46 UTC #5 lore raw

In reply to NeilBrown

Hello, Neil.

Quoted 2 lines · NeilBrown

Hi Tejun,
is this closer to what you would like?

Yes.

Quoted 8 lines

I do really need this functionality, but I honestly don't like this patch.
The need to identify just which attributes need special care seems backwards to me.
1/ it is the process (which has called mlock etc) which needs
special care, more than the attribute. Everything that process does needs to avoid allocating memory, whether that this is particularly related to enabling write-out or not.

But there are only certain things such processes can do. For example, it can't read the content of a regular file and I strongly believe that it's a bad idea to give the impression that all sysfs / kernfs files may be accessed without memory allocation.

The problem isn't just about how kernfs and sysfs are implemented. A lot of files get involved in the synchronization scheme of the subsystem that they belong to and even just telling whether a file's actual operation may end up depending on memory allocation isn't trivial at all. A file may grab a mutex inside which memory allocation happens in rare cases and we have no way of verifying and tracking them.

Also, allowing atomic access in general has risk of locking down implementation details inadvertently. Files which should have never been and don't have any need to be accessible w/o memory access could easily be forced to behave that way because some memlocked program X used it that way for convenience or whatever. Things like this are extremely painful later on when the subsystem has to be updated because there is no inherent reason whatsoever for such restriction on internal implementation.

I strongly believe we should be *very* clear to both userland and kernel ops implementations of this extra attribute.

Quoted 8 lines

2/ It is also backwards because the vast majority of sysfs
attributes only support bool/enum/int which means at most 23 chars including sign and newline (maybe more for reads if the read provides a list of options). So almost everything doesn't need an allocation at all - just some on-stack space. I would be quite happy to identify those attributes that may need care when accessing and could possibly supports read or write > 128 characters, because there wouldn't be any.

I really don't think the length has anything to do with it. Sure, we can just preallocate buffer for all open files if that's considered a good approach. I just think that the general idea of forcing all files to be allocation-free is bad.

Quoted 4 lines

I also don't like this approach because we end up allocating 2 pages for the buffer, as we have to ask for "PAGE_SIZE+1" bytes, for the hypothetical case that an important attribute actually requires a full PAGE_SIZE write...

That isn't pretty but given that only very small number of files will be marked this way, do we care?

Quoted 4 lines

Would you be happy to have all specially identified attributes be limited to 127 characters IO max? Then I would just use an on-stack buffer for those which would remove the allocation and simplify some of the code.

It's not really about the length of buffer or saving memory.

Thanks.

Signature · 1 line

tejun