Compile out splice support from mem character driver when the splice-family of
syscalls is not supported by the system (i.e. CONFIG_SYSCALL_SPLICE is
undefined).
add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-28 (-28)
function old new delta
pipe_to_null 4 - -4
splice_write_null 24 - -24
Signed-off-by: Pieter Smith <[email protected]>
---
drivers/char/mem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 917403f..420d651 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -610,6 +610,7 @@ static ssize_t aio_write_null(struct kiocb *iocb, const struct iovec *iov,
return iov_length(iov, nr_segs);
}
+#ifdef CONFIG_SYSCALL_SPLICE
static int pipe_to_null(struct pipe_inode_info *info, struct pipe_buffer *buf,
struct splice_desc *sd)
{
@@ -621,6 +622,7 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
{
return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
}
+#endif /* #ifdef CONFIG_SYSCALL_SPLICE */
static ssize_t read_zero(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
@@ -769,7 +771,7 @@ static const struct file_operations null_fops = {
.write = write_null,
.aio_read = aio_read_null,
.aio_write = aio_write_null,
- .splice_write = splice_write_null,
+ SPLICE_WRITE_INIT(splice_write_null)
};
#ifdef CONFIG_DEVPORT
--
1.9.1
On Thu, Nov 13, 2014 at 10:23:28PM +0100, Pieter Smith wrote:
> Compile out splice support from mem character driver when the splice-family of
> syscalls is not supported by the system (i.e. CONFIG_SYSCALL_SPLICE is
> undefined).
>
> add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-28 (-28)
> function old new delta
> pipe_to_null 4 - -4
> splice_write_null 24 - -24
>
> Signed-off-by: Pieter Smith <[email protected]>
> ---
> drivers/char/mem.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 917403f..420d651 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -610,6 +610,7 @@ static ssize_t aio_write_null(struct kiocb *iocb, const struct iovec *iov,
> return iov_length(iov, nr_segs);
> }
>
> +#ifdef CONFIG_SYSCALL_SPLICE
> static int pipe_to_null(struct pipe_inode_info *info, struct pipe_buffer *buf,
> struct splice_desc *sd)
> {
> @@ -621,6 +622,7 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
> {
> return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
> }
> +#endif /* #ifdef CONFIG_SYSCALL_SPLICE */
>
Ick, no, not worth the #ifdef mess, sorry.
[Please don't top-post.]
On Thu, Nov 13, 2014 at 11:23:46PM +0100, Pieter Smith wrote:
> Okay with moving the relevant functions to a new translation unit and
> squashing it out in the Makefile
No, you don't need to do that either. Mark pipe_to_null and
splice_write_null as __maybe_unused, then wrap the initialization of
.splice_write = splice_write_null to make it .splice_write =
splice_p(splice_write_null). That will avoid adding a single ifdef.
- Josh Triplett
On Thu, Nov 13, 2014 at 02:31:50PM -0800, [email protected] wrote:
> [Please don't top-post.]
>
> On Thu, Nov 13, 2014 at 11:23:46PM +0100, Pieter Smith wrote:
> > Okay with moving the relevant functions to a new translation unit and
> > squashing it out in the Makefile
>
> No, you don't need to do that either. Mark pipe_to_null and
> splice_write_null as __maybe_unused, then wrap the initialization of
> .splice_write = splice_write_null to make it .splice_write =
> splice_p(splice_write_null). That will avoid adding a single ifdef.
Again, ick, no. You aren't saving anything "real" at all, just take out
the splice core code, leave the file pointer alone, and never do that
horrid "splice_p" stuff, ick ick ick.
greg k-h
On Thu, Nov 13, 2014 at 03:34:16PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Nov 13, 2014 at 02:31:50PM -0800, [email protected] wrote:
> > [Please don't top-post.]
> >
> > On Thu, Nov 13, 2014 at 11:23:46PM +0100, Pieter Smith wrote:
> > > Okay with moving the relevant functions to a new translation unit and
> > > squashing it out in the Makefile
> >
> > No, you don't need to do that either. Mark pipe_to_null and
> > splice_write_null as __maybe_unused, then wrap the initialization of
> > .splice_write = splice_write_null to make it .splice_write =
> > splice_p(splice_write_null). That will avoid adding a single ifdef.
>
> Again, ick, no. You aren't saving anything "real" at all, just take out
> the splice core code, leave the file pointer alone, and never do that
> horrid "splice_p" stuff, ick ick ick.
Without doing the splice_p change (which should add zero lines of code,
total diffstat of -3+3 in this case, just a couple of __maybe_unused
tokens and a splice_p() in the initializer), the actual splice
implementations for filesystems and drivers won't get thrown away. I
certainly agree that #ifdefs for those would be painful and not worth
it. However, what problem would the proposed __maybe_unused / splice_p
cause?
On the other hand, I can *definitely* understand not bothering with
changing filesystems that nobody will use on a space-constrained system
(e.g. cluster filesystems); the patch series could likely be narrowed
to just a half-dozen likely filesystems and drivers, all of which could
be done separately from the initial series removing the core splice
code. Would that be more appealing?
- Josh Triplett
On Thu, Nov 13, 2014 at 04:19:48PM -0800, [email protected] wrote:
> On Thu, Nov 13, 2014 at 03:34:16PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Nov 13, 2014 at 02:31:50PM -0800, [email protected] wrote:
> > > [Please don't top-post.]
> > >
> > > On Thu, Nov 13, 2014 at 11:23:46PM +0100, Pieter Smith wrote:
> > > > Okay with moving the relevant functions to a new translation unit and
> > > > squashing it out in the Makefile
> > >
> > > No, you don't need to do that either. Mark pipe_to_null and
> > > splice_write_null as __maybe_unused, then wrap the initialization of
> > > .splice_write = splice_write_null to make it .splice_write =
> > > splice_p(splice_write_null). That will avoid adding a single ifdef.
> >
> > Again, ick, no. You aren't saving anything "real" at all, just take out
> > the splice core code, leave the file pointer alone, and never do that
> > horrid "splice_p" stuff, ick ick ick.
>
> Without doing the splice_p change (which should add zero lines of code,
> total diffstat of -3+3 in this case, just a couple of __maybe_unused
> tokens and a splice_p() in the initializer), the actual splice
> implementations for filesystems and drivers won't get thrown away. I
> certainly agree that #ifdefs for those would be painful and not worth
> it. However, what problem would the proposed __maybe_unused / splice_p
> cause?
I don't see what it buys you except churn and a constant need to keep
fixing up code that doesn't use it as new drivers get added.
It's also "not normal" when compared to all of the other function
pointers in the filesystem structure, what makes splice "special" here
that everyone now needs to know it should be treated differently?
> On the other hand, I can *definitely* understand not bothering with
> changing filesystems that nobody will use on a space-constrained system
> (e.g. cluster filesystems); the patch series could likely be narrowed
> to just a half-dozen likely filesystems and drivers, all of which could
> be done separately from the initial series removing the core splice
> code. Would that be more appealing?
As long as you aren't forcing every call-place to change, like this
patch series did, it would be better.
Also, no one ever posted how much space savings overall there was here,
so until that happens, I'm going to assume it isn't even worth the
effort, right?
thanks,
greg k-h
[Please don't top-post.]
On Sat, Nov 15, 2014 at 12:11:23AM +0100, Pieter Smith wrote:
> Off course! You are right. Most savings are gained from compiling out
> fs/splice.c. I'll eliminate file-system driver changes as far as possible.
>
> Turning splice exports into inline NOPs when splice is compiled out should
> keep me out of the file-systems.
>
> The space savings are scattered over the patch-set. I'll make sure the next
> attempt includes it in the cover-letter.
I'd suggest including the results from scripts/bloat-o-meter in the
cover letter next time.
- Josh Triplett
[Please don't top-post.]
On Sun, Nov 16, 2014 at 01:18:27PM +0100, Pieter Smith wrote:
> I had a look at the numbers (see below): For a tinyconfig kernel, savings
> outside fs/splice.c are negligible. With the addition of networking
> however, net/core and net/ipv4 can be squeezed for usable savings.
>
> Should I leave networking (and the rest) until we can come up with a
> non-icky approach outside fs/splice.c?
>
> *THE NUMBERS*
>
> Given a tinyconfig, fs/splice.c can free up 8.2K (3.5K + 4.7K) with
> CONFIG_SYSCALL_SPLICE. The rest are not worth the effort:
> fs/splice:
> syscalls only: add/remove: 0/16 grow/shrink: 2/5 up/down: 114/-3693
> (-3579)
> the remainder: add/remove: 0/24 grow/shrink: 0/4 up/down: 0/-4824
> (-4824)
Nice!
Go ahead and submit the patches for that portion, and the rest can wait
until we get compiler support for omitting structure fields.
- Josh Triplett
On Tue, Nov 18, 2014 at 10:46:59PM +0100, Pieter Smith wrote:
> Turning all exported splice functions into static inline NOP's covers
> almost everything...
> fs/fuse and net/skbuf use an exported ops struct from fs/splice.c. Mocking
> out an exported ops struct seems way uglier than linking out the
> dependencies with a __splice_p() macro and __maybe_unused.
>
> Any thoughts or suggestions?
You could make FUSE select SPLICE_SYSCALL.
For skbuff, what's the dependency? Ideally NET shouldn't select
SPLICE_SYSCALL. You might try compiling out *only* that particular
instance, and seeing how clean you can make that.
- Josh Triplett