lkml.org 
[lkml]   [2015]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 43/44] kdbus: Give up on failed fd allocation
From
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
>


\
 
 \ /
  Last update: 2015-10-08 17:41    [W:0.306 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site