Message-ID: <CAEKGpzi82ugDtCULWvFr4h19wTZMFQt6-QTUPNT_xkQWFSkbnQ@mail.gmail.com>
Date:   Sun, 15 Mar 2020 11:59:48 +0900
From:   "Daniel T. Lee" <danieltimlee@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v4 2/2] samples: bpf: refactor perf_event user
 program with libbpf bpf_link

On Sun, Mar 15, 2020 at 5:07 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Fri, Mar 13, 2020 at 8:45 PM Daniel T. Lee <danieltimlee@...il.com> wrote:
> >
> > The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
> > than the previous method using ioctl.
> >
> > bpf_program__attach_perf_event manages the enable of perf_event and
> > attach of BPF programs to it, so there's no neeed to do this
> > directly with ioctl.
> >
> > In addition, bpf_link provides consistency in the use of API because it
> > allows disable (detach, destroy) for multiple events to be treated as
> > one bpf_link__destroy. Also, bpf_link__destroy manages the close() of
> > perf_event fd.
> >
> > This commit refactors samples that attach the bpf program to perf_event
> > by using libbbpf instead of ioctl. Also the bpf_load in the samples were
> > removed and migrated to use libbbpf API.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> > ---
> > Changes in v2:
> >  - check memory allocation is successful
> >  - clean up allocated memory on error
> >
> > Changes in v3:
> >  - Improve pointer error check (IS_ERR())
> >  - change to calloc for easier destroy of bpf_link
> >  - remove perf_event fd list since bpf_link handles fd
> >  - use newer bpf_object__{open/load} API instead of bpf_prog_load
> >  - perf_event for _SC_NPROCESSORS_ONLN instead of _SC_NPROCESSORS_CONF
> >  - find program with name explicitly instead of bpf_program__next
> >  - unconditional bpf_link__destroy() on cleanup
> >
> > Changes in v4:
> >  - bpf_link *, bpf_object * set NULL on init & err for easier destroy
> >  - close bpf object with bpf_object__close()
> >
> >  samples/bpf/Makefile           |   4 +-
> >  samples/bpf/sampleip_user.c    |  98 +++++++++++++++++++----------
> >  samples/bpf/trace_event_user.c | 112 ++++++++++++++++++++++-----------
> >  3 files changed, 143 insertions(+), 71 deletions(-)
> >
>
> Few more int_exit() problems, sorry I didn't catch it first few times,
> I'm not very familiar with all these bpf samples.
>

No, you've catch the exact problem.
In previous patch, it was int_exit(error) but I've revert back on this version.
I'll explain more later.


> [...]
>
> >  all_cpu_err:
> > -       for (i--; i >= 0; i--) {
> > -               ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
> > -               close(pmu_fd[i]);
> > -       }
> > -       free(pmu_fd);
> > +       for (i--; i >= 0; i--)
> > +               bpf_link__destroy(links[i]);
> > +err:
> > +       free(links);
> >         if (error)
> >                 int_exit(0);
>
> if (error) you should exit with error, no?
>
> >  }
> >
> >  static void test_perf_event_task(struct perf_event_attr *attr)
> >  {
>
> [...]
>
> >  err:
> > -       ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> > -       close(pmu_fd);
> > +       bpf_link__destroy(link);
> >         if (error)
> >                 int_exit(0);
>
> same comment about exiting with error
>
> >  }
> > @@ -282,7 +297,9 @@ static void test_bpf_perf_event(void)
>
> [...]
>
> > @@ -305,6 +343,10 @@ int main(int argc, char **argv)
> >                 return 0;
> >         }
> >         test_bpf_perf_event();
> > +       error = 0;
> > +
> > +cleanup:
> > +       bpf_object__close(obj);
> >         int_exit(0);
>
> here and in previous sample int_exit for whatever purpose sends KILL
> signal and exits with 0, that seems weird. Any idea why it was done
> that way?
>

I'm not sure why the code was written like that previously. However, IMHO,
int_exit() is used as signal handler (not only this, other samples too)
and this function is mainly used like this.

signal(SIGINT, int_exit);

When the signal occurs, the function will receive signal number as first
parameter. So the reason why I've reverted int_exit(error) to int_exit(0) is,

Considering that this function is used as a signal handler,
one function will be used for two purposes.

static void int_exit(int sig)
{
        kill(0, SIGKILL);
        exit(0);
}

Passing error argument will make this function to indicate error on exit or
to indicate signal on exit. Also it is weird to pass extra argument which is
not signal to signal handler.

Actually when this int_exit() called, it will end before exit(0) and parent and
child process will be killed with SIGKILL and the return code will be 137,
which is 128 + 9 (SIGKILL).

# ./trace_event
# echo $?
137

One option is to remove the int_exit() that was used to terminate during the
process, not as a signal handler and change the logic to propagate the error
to main. However, this can be somewhat awkward in semantics because a
in trace_event_user.c function such as print_stacks() uses int_exit().
(Error propagated from printing stacks? not look good to me)

I not sure what is the best practice in this situation, so I just
reverted it to original.
Any ideas or suggestion is welcome.

> > -       return 0;
> > +       return error;
>
> so with that int_ext() implementation you will never get to this error
>
> >  }
> > --
> > 2.25.1
> >

Thank you for your time and effort for the review :)

Best,
Daniel

Powered by blists - more mailing lists