| Message ID | 12375b7baa741f0596d54eafc6b1cfd2489dd65a.1579553271.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | splice: direct call for default_file_splice*() | expand |
On Mon, Jan 20, 2020 at 11:49:46PM +0300, Pavel Begunkov wrote: > Indirect calls could be very expensive nowadays, so try to use direct calls > whenever possible. ... and independent of that your new version is much shorter and easier to read. But it could be improved a tiny little bit further: > if (out->f_op->splice_write) > - splice_write = out->f_op->splice_write; > + return out->f_op->splice_write(pipe, out, ppos, len, flags); > else > - splice_write = default_file_splice_write; > - > - return splice_write(pipe, out, ppos, len, flags); > + return default_file_splice_write(pipe, out, ppos, len, flags); No need for the else after an return. > if (in->f_op->splice_read) > - splice_read = in->f_op->splice_read; > + return in->f_op->splice_read(in, ppos, pipe, len, flags); > else > - splice_read = default_file_splice_read; > - > - return splice_read(in, ppos, pipe, len, flags); > + return default_file_splice_read(in, ppos, pipe, len, flags); Same here.
On 1/30/2020 7:54 PM, Christoph Hellwig wrote: > On Mon, Jan 20, 2020 at 11:49:46PM +0300, Pavel Begunkov wrote: >> Indirect calls could be very expensive nowadays, so try to use direct calls >> whenever possible. > > ... and independent of that your new version is much shorter and easier > to read. But it could be improved a tiny little bit further: > >> if (out->f_op->splice_write) >> - splice_write = out->f_op->splice_write; >> + return out->f_op->splice_write(pipe, out, ppos, len, flags); >> else >> - splice_write = default_file_splice_write; >> - >> - return splice_write(pipe, out, ppos, len, flags); >> + return default_file_splice_write(pipe, out, ppos, len, flags); > > No need for the else after an return. It generates identical binary. For this to not look sloppy, I'd add new line between, so the same 4 lines. And this looks better for me, but that's rather subjective. I don't think it's worth of changing. What's the benefit? > >> if (in->f_op->splice_read) >> - splice_read = in->f_op->splice_read; >> + return in->f_op->splice_read(in, ppos, pipe, len, flags); >> else >> - splice_read = default_file_splice_read; >> - >> - return splice_read(in, ppos, pipe, len, flags); >> + return default_file_splice_read(in, ppos, pipe, len, flags); > > Same here. >
On 30/01/2020 19:54, Christoph Hellwig wrote: > On Mon, Jan 20, 2020 at 11:49:46PM +0300, Pavel Begunkov wrote: >> Indirect calls could be very expensive nowadays, so try to use direct calls >> whenever possible. Hah, I'm surprised to find it as 00c285d0d0fe4 ("fs: simplify do_splice_from"). Christoph, even though this one is not a big deal, I'm finding the practice of taking others patches and silently sending them as yours own in general disgusting. Just for you to know. > > ... and independent of that your new version is much shorter and easier > to read. But it could be improved a tiny little bit further: > >> if (out->f_op->splice_write) >> - splice_write = out->f_op->splice_write; >> + return out->f_op->splice_write(pipe, out, ppos, len, flags); >> else >> - splice_write = default_file_splice_write; >> - >> - return splice_write(pipe, out, ppos, len, flags); >> + return default_file_splice_write(pipe, out, ppos, len, flags); > > No need for the else after an return. > >> if (in->f_op->splice_read) >> - splice_read = in->f_op->splice_read; >> + return in->f_op->splice_read(in, ppos, pipe, len, flags); >> else >> - splice_read = default_file_splice_read; >> - >> - return splice_read(in, ppos, pipe, len, flags); >> + return default_file_splice_read(in, ppos, pipe, len, flags); > > Same here. >
On Sat, Aug 01, 2020 at 01:12:22PM +0300, Pavel Begunkov wrote: > On 30/01/2020 19:54, Christoph Hellwig wrote: > > On Mon, Jan 20, 2020 at 11:49:46PM +0300, Pavel Begunkov wrote: > >> Indirect calls could be very expensive nowadays, so try to use direct calls > >> whenever possible. > > Hah, I'm surprised to find it as > 00c285d0d0fe4 ("fs: simplify do_splice_from"). > > Christoph, even though this one is not a big deal, I'm finding the > practice of taking others patches and silently sending them as yours > own in general disgusting. Just for you to know. Err, what makes you think I took your patch vs just not remembering and pointlessly doing the same cleanup again? If I had rembered your patch I would have just added to the series with your credit as I've done for plenty other patches..
On 01/08/2020 20:41, Christoph Hellwig wrote: > On Sat, Aug 01, 2020 at 01:12:22PM +0300, Pavel Begunkov wrote: >> On 30/01/2020 19:54, Christoph Hellwig wrote: >>> On Mon, Jan 20, 2020 at 11:49:46PM +0300, Pavel Begunkov wrote: >>>> Indirect calls could be very expensive nowadays, so try to use direct calls >>>> whenever possible. >> >> Hah, I'm surprised to find it as >> 00c285d0d0fe4 ("fs: simplify do_splice_from"). >> >> Christoph, even though this one is not a big deal, I'm finding the >> practice of taking others patches and silently sending them as yours >> own in general disgusting. Just for you to know. > > Err, what makes you think I took your patch vs just not remembering > and pointlessly doing the same cleanup again? If I had rembered your > patch I would have just added to the series with your credit as I've > done for plenty other patches.. I have no intention of picking it to pieces or something, it doesn't worth our time, and glad that's by accident, but you may guess how it looks -- you commented on it, and after not being picked, the patch reappears slightly rebranded.
diff --git a/fs/splice.c b/fs/splice.c index 6a6f30432688..91448d855ff0 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -852,15 +852,10 @@ EXPORT_SYMBOL(generic_splice_sendpage); static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, loff_t *ppos, size_t len, unsigned int flags) { - ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, - loff_t *, size_t, unsigned int); - if (out->f_op->splice_write) - splice_write = out->f_op->splice_write; + return out->f_op->splice_write(pipe, out, ppos, len, flags); else - splice_write = default_file_splice_write; - - return splice_write(pipe, out, ppos, len, flags); + return default_file_splice_write(pipe, out, ppos, len, flags); } /* @@ -870,8 +865,6 @@ static long do_splice_to(struct file *in, loff_t *ppos, struct pipe_inode_info *pipe, size_t len, unsigned int flags) { - ssize_t (*splice_read)(struct file *, loff_t *, - struct pipe_inode_info *, size_t, unsigned int); int ret; if (unlikely(!(in->f_mode & FMODE_READ))) @@ -885,11 +878,9 @@ static long do_splice_to(struct file *in, loff_t *ppos, len = MAX_RW_COUNT; if (in->f_op->splice_read) - splice_read = in->f_op->splice_read; + return in->f_op->splice_read(in, ppos, pipe, len, flags); else - splice_read = default_file_splice_read; - - return splice_read(in, ppos, pipe, len, flags); + return default_file_splice_read(in, ppos, pipe, len, flags); } /**
Indirect calls could be very expensive nowadays, so try to use direct calls whenever possible. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/splice.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)