Messages in this thread | | | Date | Thu, 8 Oct 2015 17:14:24 +0200 | Subject | Re: [PATCH 43/44] kdbus: Give up on failed fd allocation | From | David Herrmann <> |
| |
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.
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 >
| |