Messages in this thread | | | Date | Fri, 9 Oct 2015 21:49:41 +0300 | From | Sergei Zviagintsev <> | Subject | Re: [PATCH 43/44] kdbus: Give up on failed fd allocation |
| |
Hi,
On Thu, Oct 08, 2015 at 05:14:24PM +0200, David Herrmann wrote: > Hi > > On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@s15v.net> wrote: > > If we failed to allocate a file descriptor, do not try to do it again on > > the next iteration. There are few chances that we will have success, and > > we can simplify the algorithm assuming that valid fd numbers are not > > mixed with -1 values. > > Why? This is no a fast-path, and the penalty is accounted on the > actual culprit (the caller). I don't see why we should optimize for > that case. If the fd-allocation fails, you clearly did something > wrong.
I agree, but this is optimization for readability and not for speed. If we don't care whether fd-allocation failed or not, why should we keep less readable variant which handles those -1 values mixed with valid fds numbers?
> > Thanks > David > > > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net> > > --- > > ipc/kdbus/message.c | 42 ++++++++++++++++++++++++------------------ > > 1 file changed, 24 insertions(+), 18 deletions(-) > > > > diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c > > index da685049d66c..75e6213e7ed5 100644 > > --- a/ipc/kdbus/message.c > > +++ b/ipc/kdbus/message.c > > @@ -123,7 +123,7 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice, > > { > > bool incomplete_fds = false; > > struct kvec kvec; > > - size_t i, n_fds; > > + size_t i, n_fds, n_memfds; > > int ret, *fds; > > > > if (!gaps) { > > @@ -140,25 +140,31 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice, > > } > > > > fds = kmalloc_array(n_fds, sizeof(*fds), GFP_TEMPORARY); > > - n_fds = 0; > > if (!fds) > > return -ENOMEM; > > > > /* 1) allocate fds and copy them over */ > > > > + n_fds = 0; /* n_fds now tracks the number of allocated fds */ > > if (gaps->n_fds > 0) { > > for (i = 0; i < gaps->n_fds; ++i) { > > int fd; > > > > fd = get_unused_fd_flags(O_CLOEXEC); > > - if (fd < 0) > > + if (fd < 0) { > > incomplete_fds = true; > > + break; > > + } > > > > WARN_ON(!gaps->fd_files[i]); > > > > - fds[n_fds++] = fd < 0 ? -1 : fd; > > + fds[n_fds++] = fd; > > } > > > > + /* If we couldn't allocate a fd, fill the rest with -1 */ > > + for ( ; i < gaps->n_fds; ++i) > > + fds[i] = -1; > > + > > /* > > * The file-descriptor array can only be present once per > > * message. Hence, prepare all fds and then copy them over with > > @@ -175,18 +181,18 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice, > > goto exit; > > } > > > > - for (i = 0; i < gaps->n_memfds; ++i) { > > + n_memfds = 0; > > + for (i = 0; i < gaps->n_memfds && !incomplete_fds; ++i) { > > int memfd; > > > > memfd = get_unused_fd_flags(O_CLOEXEC); > > if (memfd < 0) { > > incomplete_fds = true; > > - fds[n_fds++] = -1; > > /* memfds are initialized to -1, skip copying it */ > > - continue; > > + break; > > } > > > > - fds[n_fds++] = memfd; > > + fds[gaps->n_fds + n_memfds++] = memfd; > > > > /* > > * memfds have to be copied individually as they each are put > > @@ -208,21 +214,21 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice, > > > > /* 2) install fds now that everything was successful */ > > > > - for (i = 0; i < gaps->n_fds; ++i) > > - if (fds[i] >= 0) > > - fd_install(fds[i], get_file(gaps->fd_files[i])); > > - for (i = 0; i < gaps->n_memfds; ++i) > > - if (fds[gaps->n_fds + i] >= 0) > > - fd_install(fds[gaps->n_fds + i], > > - get_file(gaps->memfd_files[i])); > > + for (i = 0; i < n_fds; ++i) > > + fd_install(fds[i], get_file(gaps->fd_files[i])); > > + for (i = 0; i < n_memfds; ++i) > > + fd_install(fds[gaps->n_fds + i], > > + get_file(gaps->memfd_files[i])); > > > > ret = 0; > > > > exit: > > - if (ret < 0) > > + if (ret < 0) { > > for (i = 0; i < n_fds; ++i) > > - if (fds[i] >= 0) > > - put_unused_fd(fds[i]); > > + put_unused_fd(fds[i]); > > + for (i = 0; i < n_memfds; ++i) > > + put_unused_fd(fds[gaps->n_fds + i]); > > + } > > kfree(fds); > > *out_incomplete = incomplete_fds; > > return ret; > > -- > > 1.8.3.1 > >
| |