Messages in this thread | | | Subject | Re: [PATCH] perf record: Fix continue profiling after draining the buffer | From | Yang Jihong <> | Date | Mon, 22 Feb 2021 09:31:51 +0800 |
| |
Hello,
On 2021/2/19 1:09, Arnaldo Carvalho de Melo wrote: > Em Thu, Feb 18, 2021 at 10:20:53AM -0300, Arnaldo Carvalho de Melo escreveu: >> Em Fri, Feb 05, 2021 at 11:46:12AM +0100, Jiri Olsa escreveu: >>> On Fri, Feb 05, 2021 at 07:35:22PM +0900, Namhyung Kim wrote: >>>> Hello, >>>> >>>> On Fri, Feb 5, 2021 at 3:50 PM Yang Jihong <yangjihong1@huawei.com> wrote: >>>>> >>>>> commit da231338ec9c098707c8a1e4d8a50e2400e2fe17 uses eventfd to solve rare race >>>>> where the setting and checking of 'done' which add done_fd to pollfd. >>>>> When draining buffer, revents of done_fd is 0 and evlist__filter_pollfd >>>>> function returns a non-zero value. >>>>> As a result, perf record does not stop profiling. >>>>> >>>>> The following simple scenarios can trigger this condition: >>>>> >>>>> sleep 10 & >>>>> perf record -p $! >>>>> >>>>> After the sleep process exits, perf record should stop profiling and exit. >>>>> However, perf record keeps running. >>>>> >>>>> If pollfd revents contains only POLLERR or POLLHUP, >>>>> perf record indicates that buffer is draining and need to stop profiling. >>>>> Use fdarray_flag__nonfilterable to set done eventfd to nonfilterable objects, >>>>> so that evlist__filter_pollfd does not filter and check done eventfd. >>>>> >>>>> Fixes: da231338ec9c (perf record: Use an eventfd to wakeup when done) >>>>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> >>>>> --- >>>>> tools/perf/builtin-record.c | 2 +- >>>>> tools/perf/util/evlist.c | 8 ++++++++ >>>>> tools/perf/util/evlist.h | 4 ++++ >>>>> 3 files changed, 13 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >>>>> index fd3911650612..51e593e896ea 100644 >>>>> --- a/tools/perf/builtin-record.c >>>>> +++ b/tools/perf/builtin-record.c >>>>> @@ -1663,7 +1663,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) >>>>> status = -1; >>>>> goto out_delete_session; >>>>> } >>>>> - err = evlist__add_pollfd(rec->evlist, done_fd); >>>>> + err = evlist__add_wakeup_eventfd(rec->evlist, done_fd); >>>>> if (err < 0) { >>>>> pr_err("Failed to add wakeup eventfd to poll list\n"); >>>>> status = err; >>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >>>>> index 05363a7247c4..fea4c1e8010d 100644 >>>>> --- a/tools/perf/util/evlist.c >>>>> +++ b/tools/perf/util/evlist.c >>>>> @@ -572,6 +572,14 @@ int evlist__filter_pollfd(struct evlist *evlist, short revents_and_mask) >>>>> return perf_evlist__filter_pollfd(&evlist->core, revents_and_mask); >>>>> } >>>>> >>>>> +#ifdef HAVE_EVENTFD_SUPPORT >>>>> +int evlist__add_wakeup_eventfd(struct evlist *evlist, int fd) >>>>> +{ >>>>> + return perf_evlist__add_pollfd(&evlist->core, fd, NULL, POLLIN, >>>>> + fdarray_flag__nonfilterable); >>>>> +} >>>>> +#endif >>>> >>>> Does it build when HAVE_EVENTFD_SUPPORT is not defined? >>> >>> yea, I was wondering the same.. but it's called only from >>> code within HAVE_EVENTFD_SUPPORT ifdef >> >> Yes, this can't work on systems without eventfd, it will simply not >> build, and why do we have to make the definition of this function >> conditional on HAVE_EVENTFD_SUPPORT? >> >> I'm missing something :-\ >> >> Yeah, this whole call to evlist__add_pollfd is already surrounded by >> #ifdef HAVE_EVENTFD_SUPPORT: >> >> 1656 if (zstd_init(&session->zstd_data, rec->opts.comp_level) < 0) { >> 1657 pr_err("Compression initialization failed.\n"); >> 1658 return -1; >> 1659 } >> 1660 #ifdef HAVE_EVENTFD_SUPPORT >> 1661 done_fd = eventfd(0, EFD_NONBLOCK); >> 1662 if (done_fd < 0) { >> 1663 pr_err("Failed to create wakeup eventfd, error: %m\n"); >> 1664 status = -1; >> 1665 goto out_delete_session; >> 1666 } >> 1667 err = evlist__add_pollfd(rec->evlist, done_fd); >> 1668 if (err < 0) { >> 1669 pr_err("Failed to add wakeup eventfd to poll list\n"); >> 1670 status = err; >> 1671 goto out_delete_session; >> 1672 } >> 1673 #endif // HAVE_EVENTFD_SUPPORT >> 1674 >> 1675 session->header.env.comp_type = PERF_COMP_ZSTD; >> 1676 session->header.env.comp_level = rec->opts.comp_level; >> >> Jiri, does your Acked-by stands? Namhyung? > > Thanks tested and applied, together with Jiri's Tested-by, > > - Arnaldo > . > Is this patch okay? Is there anything that needs to be modified?
Thanks, Yang
| |