[lttng-dev] [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

[Jérémie Galarneau]

Thanks for this patch, it does allow a worthwhile clean-up!
Read on for comments about style.

On Thu, 21 Mar 2019 at 11:05, Yannick Lamarre <ylamarre at efficios.com> wrote:
>
> This removes the need to verify for eventless file descriptors and
> mitigates risks of bug due to behaviour mismatch.
>
> Signed-off-by: Yannick Lamarre <ylamarre at efficios.com>
> ---
>  src/common/compat/compat-poll.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index b7c17df7..6aa9414d 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -292,6 +292,7 @@ error:
>  int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>  {
>         int ret;
> +       int eidx = 0;

Reading the code, this seems to mean the "index of the first idle file
descriptor".

As a general rule, don't use uncommon abbreviations. Abbreviations
are okay when they are common-place, unambiguous and save
a lot of characters (e.g. cfg for configuration, or lttng for
linux_trace_toolkit_next_generation).

When in doubt, prefer verbose names.

In this case, "idle_pfd_index" and a comment (see below) would
clarify the intent.

>
>         if (events == NULL || events->current.events == NULL) {
>                 ERR("poll wait arguments error");
> @@ -324,10 +325,36 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
>         }
>
>         /*
> -        * poll() should always iterate on all FDs since we handle the pollset in
> -        * user space and after poll returns, we have to try every fd for a match.
> +        * Swap all nonzero revents pollfd structs to the beginning of the array to
> +        * emulate compat-epoll behaviour. This algorithm takes advantage of poll's
> +        * returned value and the burst nature of waking events on the file
> +        * descriptors. The while loop garantees that idlepfd will always point to

garantees -> guarantees

This comment block exceeds 80 chars. Make sure your editor's tab width is set
to 8 characters.

The code, commit message, and comments make use of multiple terms
to refer to the same file descriptor state ('idle', 'empty',
'eventless', and 'zero').

Please stick to one of those terms.

Idle seems preferable, but feel free to whichever term the poll() specification
uses.

> +        * a pollfd with revents == 0.
>          */
> -       return events->wait.nb_fd;
> +       if (ret == events->wait.nb_fd) {
> +               goto skip;
> +       }
> +       while (eidx < ret && events->wait.events[eidx].revents != 0) {
> +               eidx += 1;

Prefer "eidx++".

Using variables for a single purpose is fine and tends to make the
code clearer.

For example, reworking this section of code to:

active_fd_count = ret;
/* Find the first idle pollfd. */
while (idle_pfd_index < active_fd_count &&
events->wait.events[idle_pfd_index].revents != 0) {
        idle_pfd_index++;
}

makes the intent clearer.

> +       }
> +
> +       for (int idx = eidx + 1; eidx < ret; idx++) {

Same change here (regarding 'ret').

To maintain uniformity with the rest of the code base, please declare
variables at the top of the inner-most scope containing the loop.

When declaring a trivial loop counter/index, use the name 'i'. Here, the
difference between 'idx' and 'eidx' is not immediately apparent.
Renaming 'eidx' makes the loop clearer.

Also, in this particular case, 'i' is fine since you are only using it to
assign the current pollfd, which is fairly descriptive.

> +               struct pollfd swapfd;
> +               struct pollfd *idlepfd = &events->wait.events[eidx];
> +               struct pollfd *ipfd = &events->wait.events[idx];

Use an underscore to split variable names, e.g. swap_pfd, idle_pfd,
current_pfd. Again, verbose names are preferable.

> +
> +               assert(idx < events->wait.nb_fd);

Not needed. Using assert() is fine to highlight assumptions across
interfaces/functions and/or catch catastrophic internal errors. Here,
this would be a very local condition that seems to be already
enforced.

Reworking the 'for' condition to use 'i < events->wait.nb_fd' and
breaking out early of the loop is also an option here.

> +
> +               if (ipfd->revents != 0) {
> +                       swapfd = *ipfd;
> +                       *ipfd = *idlepfd;
> +                       *idlepfd = swapfd;
> +                       eidx += 1;

eidx++;

> +               }
> +       }
> +
> +skip:

Rename label to 'end'.

Thanks!
Jérémie


> +       return ret;
>
>  error:
>         return -1;
> --
> 2.11.0
>


--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com