Re: [PATCH 43/44] kdbus: Give up on failed fd allocation

[Sergei Zviagintsev <sergei@s15v.net>]

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
> >