Date:	Thu, 8 Oct 2015 17:14:24 +0200
From:	David Herrmann <dh.herrmann@...il.com>
To:	Sergei Zviagintsev <sergei@...v.net>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	Djalal Harouni <tixxdz@...ndz.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 43/44] kdbus: Give up on failed fd allocation

Hi

On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@...v.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@...v.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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists