From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
To: Tomasz Duszynski <tduszynski@marvell.com>,
Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [EXT] Re: [PATCH v11 1/4] lib: add generic support for reading PMU events
Date: Tue, 28 Feb 2023 11:35:48 +0000 [thread overview]
Message-ID: <fdcdeb8db0e04faea8e6b92542bd7306@huawei.com> (raw)
In-Reply-To: <DM4PR18MB4368B434182FD854FF8B6B7CD2AF9@DM4PR18MB4368.namprd18.prod.outlook.com>
> >>>>>>>>>> diff --git a/lib/pmu/rte_pmu.h b/lib/pmu/rte_pmu.h new file
> >>>>>>>>>> mode
> >>>>>>>>>> 100644 index 0000000000..6b664c3336
> >>>>>>>>>> --- /dev/null
> >>>>>>>>>> +++ b/lib/pmu/rte_pmu.h
> >>>>>>>>>> @@ -0,0 +1,212 @@
> >>>>>>>>>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>>>>>>>>> + * Copyright(c) 2023 Marvell */
> >>>>>>>>>> +
> >>>>>>>>>> +#ifndef _RTE_PMU_H_
> >>>>>>>>>> +#define _RTE_PMU_H_
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * @file
> >>>>>>>>>> + *
> >>>>>>>>>> + * PMU event tracing operations
> >>>>>>>>>> + *
> >>>>>>>>>> + * This file defines generic API and types necessary to setup
> >>>>>>>>>> +PMU and
> >>>>>>>>>> + * read selected counters in runtime.
> >>>>>>>>>> + */
> >>>>>>>>>> +
> >>>>>>>>>> +#ifdef __cplusplus
> >>>>>>>>>> +extern "C" {
> >>>>>>>>>> +#endif
> >>>>>>>>>> +
> >>>>>>>>>> +#include <linux/perf_event.h>
> >>>>>>>>>> +
> >>>>>>>>>> +#include <rte_atomic.h>
> >>>>>>>>>> +#include <rte_branch_prediction.h> #include <rte_common.h>
> >>>>>>>>>> +#include <rte_compat.h> #include <rte_spinlock.h>
> >>>>>>>>>> +
> >>>>>>>>>> +/** Maximum number of events in a group */ #define
> >>>>>>>>>> +MAX_NUM_GROUP_EVENTS 8
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * A structure describing a group of events.
> >>>>>>>>>> + */
> >>>>>>>>>> +struct rte_pmu_event_group {
> >>>>>>>>>> + struct perf_event_mmap_page
> >>>>>>>>>> +*mmap_pages[MAX_NUM_GROUP_EVENTS];
> >>>>>>>>>> +/**< array of user pages
> >>>>>>> */
> >>>>>>>>>> + int fds[MAX_NUM_GROUP_EVENTS]; /**< array of event descriptors */
> >>>>>>>>>> + bool enabled; /**< true if group was enabled on particular lcore */
> >>>>>>>>>> + TAILQ_ENTRY(rte_pmu_event_group) next; /**< list entry */ }
> >>>>>>>>>> +__rte_cache_aligned;
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * A structure describing an event.
> >>>>>>>>>> + */
> >>>>>>>>>> +struct rte_pmu_event {
> >>>>>>>>>> + char *name; /**< name of an event */
> >>>>>>>>>> + unsigned int index; /**< event index into fds/mmap_pages */
> >>>>>>>>>> + TAILQ_ENTRY(rte_pmu_event) next; /**< list entry */ };
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * A PMU state container.
> >>>>>>>>>> + */
> >>>>>>>>>> +struct rte_pmu {
> >>>>>>>>>> + char *name; /**< name of core PMU listed under /sys/bus/event_source/devices */
> >>>>>>>>>> + rte_spinlock_t lock; /**< serialize access to event group list */
> >>>>>>>>>> + TAILQ_HEAD(, rte_pmu_event_group) event_group_list; /**< list of event groups */
> >>>>>>>>>> + unsigned int num_group_events; /**< number of events in a group */
> >>>>>>>>>> + TAILQ_HEAD(, rte_pmu_event) event_list; /**< list of matching events */
> >>>>>>>>>> + unsigned int initialized; /**< initialization counter */ };
> >>>>>>>>>> +
> >>>>>>>>>> +/** lcore event group */
> >>>>>>>>>> +RTE_DECLARE_PER_LCORE(struct rte_pmu_event_group,
> >>>>>>>>>> +_event_group);
> >>>>>>>>>> +
> >>>>>>>>>> +/** PMU state container */
> >>>>>>>>>> +extern struct rte_pmu rte_pmu;
> >>>>>>>>>> +
> >>>>>>>>>> +/** Each architecture supporting PMU needs to provide its own
> >>>>>>>>>> +version */ #ifndef rte_pmu_pmc_read #define
> >>>>>>>>>> +rte_pmu_pmc_read(index) ({ 0; }) #endif
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * @warning
> >>>>>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice
> >>>>>>>>>> + *
> >>>>>>>>>> + * Read PMU counter.
> >>>>>>>>>> + *
> >>>>>>>>>> + * @warning This should be not called directly.
> >>>>>>>>>> + *
> >>>>>>>>>> + * @param pc
> >>>>>>>>>> + * Pointer to the mmapped user page.
> >>>>>>>>>> + * @return
> >>>>>>>>>> + * Counter value read from hardware.
> >>>>>>>>>> + */
> >>>>>>>>>> +static __rte_always_inline uint64_t
> >>>>>>>>>> +__rte_pmu_read_userpage(struct perf_event_mmap_page *pc) {
> >>>>>>>>>> + uint64_t width, offset;
> >>>>>>>>>> + uint32_t seq, index;
> >>>>>>>>>> + int64_t pmc;
> >>>>>>>>>> +
> >>>>>>>>>> + for (;;) {
> >>>>>>>>>> + seq = pc->lock;
> >>>>>>>>>> + rte_compiler_barrier();
> >>>>>>>>>
> >>>>>>>>> Are you sure that compiler_barrier() is enough here?
> >>>>>>>>> On some archs CPU itself has freedom to re-order reads.
> >>>>>>>>> Or I am missing something obvious here?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> It's a matter of not keeping old stuff cached in registers and
> >>>>>>>> making sure that we have two reads of lock. CPU reordering won't
> >>>>>>>> do any harm here.
> >>>>>>>
> >>>>>>> Sorry, I didn't get you here:
> >>>>>>> Suppose CPU will re-order reads and will read lock *after* index or offset value.
> >>>>>>> Wouldn't it mean that in that case index and/or offset can contain old/invalid values?
> >>>>>>>
> >>>>>>
> >>>>>> This number is just an indicator whether kernel did change something or not.
> >>>>>
> >>>>> You are talking about pc->lock, right?
> >>>>> Yes, I do understand that it is sort of seqlock.
> >>>>> That's why I am puzzled why we do not care about possible cpu read-reordering.
> >>>>> Manual for perf_event_open() also has a code snippet with compiler barrier only...
> >>>>>
> >>>>>> If cpu reordering will come into play then this will not change
> >>>>>> anything from pov of this
> >>> loop.
> >>>>>> All we want is fresh data when needed and no involvement of
> >>>>>> compiler when it comes to reordering code.
> >>>>>
> >>>>> Ok, can you probably explain to me why the following could not happen:
> >>>>> T0:
> >>>>> pc->seqlock==0; pc->index==I1; pc->offset==O1;
> >>>>> T1:
> >>>>> cpu #0 read pmu (due to cpu read reorder, we get index value before seqlock):
> >>>>> index=pc->index; //index==I1;
> >>>>> T2:
> >>>>> cpu #1 kernel vent_update_userpage:
> >>>>> pc->lock++; // pc->lock==1
> >>>>> pc->index=I2;
> >>>>> pc->offset=O2;
> >>>>> ...
> >>>>> pc->lock++; //pc->lock==2
> >>>>> T3:
> >>>>> cpu #0 continue with read pmu:
> >>>>> seq=pc->lock; //seq == 2
> >>>>> offset=pc->offset; // offset == O2
> >>>>> ....
> >>>>> pmc = rte_pmu_pmc_read(index - 1); // Note that we read at I1, not I2
> >>>>> offset += pmc; //offset == O2 + pmcread(I1-1);
> >>>>> if (pc->lock == seq) // they are equal, return
> >>>>> return offset;
> >>>>>
> >>>>> Or, it can happen, but by some reason we don't care much?
> >>>>>
> >>>>
> >>>> This code does self-monitoring and user page (whole group actually)
> >>>> is per thread running on current cpu. Hence I am not sure what are
> >>>> you trying to prove with that
> >>> example.
> >>>
> >>> I am not trying to prove anything so far.
> >>> I am asking is such situation possible or not, and if not, why?
> >>> My current understanding (possibly wrong) is that after you mmaped
> >>> these pages, kernel still can asynchronously update them.
> >>> So, when reading the data from these pages you have to check 'lock'
> >>> value before and after accessing other data.
> >>> If so, why possible cpu read-reordering doesn't matter?
> >>>
> >>
> >> Look. I'll reiterate that.
> >>
> >> 1. That user page/group/PMU config is per process. Other processes do not access that.
> >
> >Ok, that's clear.
> >
> >
> >> All this happens on the very same CPU where current thread is running.
> >
> >Ok... but can't this page be updated by kernel thread running simultaneously on different CPU?
> >
>
> I already pointed out that event/counter configuration is bound to current cpu. How can possibly
> other cpu update that configuration? This cannot work.
Can you elaborate a bit what you mean with 'event/counter configuration is bound to current cpu'?
If that means it could be updated only by code running on given, CPU - yes it is clear.
But can this page be read by user-space from different CPU?
Or you just assume that your user-space thread will *always* be bounded just to one
particular CPU and would never switch?
>
>
> If you think that there's some problem with the code (or is simply broken on your setup) and logic[] a bit
> has obvious flaw and you can provide meaningful evidence of that then I'd be more than happy to
> apply that fix. Otherwise that discussion will get us nowhere.
>
Yes, we are going in cycles here.
I keep asking you same questions about library function internals, you keep refusing to explain things
to me insisting that it is 'way too obvious'.
Well, sorry but it is not obvious to me.
So I still insist that a clearly documented internal design and expected usage is required for that patch
before it can be accepted.
Konstantin
next prev parent reply other threads:[~2023-02-28 11:35 UTC|newest]
Thread overview: 139+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-11 9:43 [PATCH 0/4] add support for self monitoring Tomasz Duszynski
2022-11-11 9:43 ` [PATCH 1/4] eal: add generic support for reading PMU events Tomasz Duszynski
2022-12-15 8:33 ` Mattias Rönnblom
2022-11-11 9:43 ` [PATCH 2/4] eal/arm: support reading ARM PMU events in runtime Tomasz Duszynski
2022-11-11 9:43 ` [PATCH 3/4] eal/x86: support reading Intel " Tomasz Duszynski
2022-11-11 9:43 ` [PATCH 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2022-11-21 12:11 ` [PATCH v2 0/4] add support for self monitoring Tomasz Duszynski
2022-11-21 12:11 ` [PATCH v2 1/4] eal: add generic support for reading PMU events Tomasz Duszynski
2022-11-21 12:11 ` [PATCH v2 2/4] eal/arm: support reading ARM PMU events in runtime Tomasz Duszynski
2022-11-21 12:11 ` [PATCH v2 3/4] eal/x86: support reading Intel " Tomasz Duszynski
2022-11-21 12:11 ` [PATCH v2 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2022-11-29 9:28 ` [PATCH v3 0/4] add support for self monitoring Tomasz Duszynski
2022-11-29 9:28 ` [PATCH v3 1/4] eal: add generic support for reading PMU events Tomasz Duszynski
2022-11-30 8:32 ` zhoumin
2022-12-13 8:05 ` [EXT] " Tomasz Duszynski
2022-11-29 9:28 ` [PATCH v3 2/4] eal/arm: support reading ARM PMU events in runtime Tomasz Duszynski
2022-11-29 9:28 ` [PATCH v3 3/4] eal/x86: support reading Intel " Tomasz Duszynski
2022-11-29 9:28 ` [PATCH v3 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2022-11-29 10:42 ` [PATCH v3 0/4] add support for self monitoring Morten Brørup
2022-12-13 8:23 ` Tomasz Duszynski
2022-12-13 10:43 ` [PATCH v4 " Tomasz Duszynski
2022-12-13 10:43 ` [PATCH v4 1/4] eal: add generic support for reading PMU events Tomasz Duszynski
2022-12-13 11:52 ` Morten Brørup
2022-12-14 9:38 ` Tomasz Duszynski
2022-12-14 10:41 ` Morten Brørup
2022-12-15 8:22 ` Morten Brørup
2022-12-16 7:33 ` Morten Brørup
2023-01-05 21:14 ` Tomasz Duszynski
2023-01-05 22:07 ` Morten Brørup
2023-01-08 15:41 ` Tomasz Duszynski
2023-01-08 16:30 ` Morten Brørup
2022-12-15 8:46 ` Mattias Rönnblom
2023-01-04 15:47 ` Tomasz Duszynski
2023-01-09 7:37 ` Ruifeng Wang
2023-01-09 15:40 ` Tomasz Duszynski
2022-12-13 10:43 ` [PATCH v4 2/4] eal/arm: support reading ARM PMU events in runtime Tomasz Duszynski
2022-12-13 10:43 ` [PATCH v4 3/4] eal/x86: support reading Intel " Tomasz Duszynski
2022-12-13 10:43 ` [PATCH v4 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-01-10 23:46 ` [PATCH v5 0/4] add support for self monitoring Tomasz Duszynski
2023-01-10 23:46 ` [PATCH v5 1/4] eal: add generic support for reading PMU events Tomasz Duszynski
2023-01-11 9:05 ` Morten Brørup
2023-01-11 16:20 ` Tomasz Duszynski
2023-01-11 16:54 ` Morten Brørup
2023-01-10 23:46 ` [PATCH v5 2/4] eal/arm: support reading ARM PMU events in runtime Tomasz Duszynski
2023-01-10 23:46 ` [PATCH v5 3/4] eal/x86: support reading Intel " Tomasz Duszynski
2023-01-10 23:46 ` [PATCH v5 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-01-11 0:32 ` [PATCH v5 0/4] add support for self monitoring Tyler Retzlaff
2023-01-11 9:31 ` Morten Brørup
2023-01-11 14:24 ` Tomasz Duszynski
2023-01-11 14:32 ` Bruce Richardson
2023-01-11 9:39 ` [EXT] " Tomasz Duszynski
2023-01-11 21:05 ` Tyler Retzlaff
2023-01-13 7:44 ` Tomasz Duszynski
2023-01-13 19:22 ` Tyler Retzlaff
2023-01-14 9:53 ` Morten Brørup
2023-01-19 23:39 ` [PATCH v6 " Tomasz Duszynski
2023-01-19 23:39 ` [PATCH v6 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-01-20 9:46 ` Morten Brørup
2023-01-26 9:40 ` Tomasz Duszynski
2023-01-26 12:29 ` Morten Brørup
2023-01-26 12:59 ` Bruce Richardson
2023-01-26 15:28 ` [EXT] " Tomasz Duszynski
2023-02-02 14:27 ` Morten Brørup
2023-01-26 15:17 ` Tomasz Duszynski
2023-01-20 18:29 ` Tyler Retzlaff
2023-01-26 9:05 ` [EXT] " Tomasz Duszynski
2023-01-19 23:39 ` [PATCH v6 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-01-19 23:39 ` [PATCH v6 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-01-19 23:39 ` [PATCH v6 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-01 13:17 ` [PATCH v7 0/4] add support for self monitoring Tomasz Duszynski
2023-02-01 13:17 ` [PATCH v7 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-02-01 13:17 ` [PATCH v7 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-02-01 13:17 ` [PATCH v7 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-02-01 13:17 ` [PATCH v7 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-01 13:51 ` [PATCH v7 0/4] add support for self monitoring Morten Brørup
2023-02-02 7:54 ` Tomasz Duszynski
2023-02-02 9:43 ` [PATCH v8 " Tomasz Duszynski
2023-02-02 9:43 ` [PATCH v8 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-02-02 10:32 ` Ruifeng Wang
2023-02-02 9:43 ` [PATCH v8 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-02-02 9:43 ` [PATCH v8 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-02-02 9:43 ` [PATCH v8 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-02 12:49 ` [PATCH v9 0/4] add support for self monitoring Tomasz Duszynski
2023-02-02 12:49 ` [PATCH v9 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-02-06 11:02 ` David Marchand
2023-02-09 11:09 ` [EXT] " Tomasz Duszynski
2023-02-02 12:49 ` [PATCH v9 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-02-02 12:49 ` [PATCH v9 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-02-02 12:49 ` [PATCH v9 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-13 11:31 ` [PATCH v10 0/4] add support for self monitoring Tomasz Duszynski
2023-02-13 11:31 ` [PATCH v10 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-02-16 7:39 ` Ruifeng Wang
2023-02-16 14:44 ` Tomasz Duszynski
2023-02-13 11:31 ` [PATCH v10 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-02-16 7:41 ` Ruifeng Wang
2023-02-13 11:31 ` [PATCH v10 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-02-13 11:31 ` [PATCH v10 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-16 17:54 ` [PATCH v11 0/4] add support for self monitoring Tomasz Duszynski
2023-02-16 17:54 ` [PATCH v11 1/4] lib: add generic support for reading PMU events Tomasz Duszynski
2023-02-16 23:50 ` Konstantin Ananyev
2023-02-17 8:49 ` [EXT] " Tomasz Duszynski
2023-02-17 10:14 ` Konstantin Ananyev
2023-02-19 14:23 ` Tomasz Duszynski
2023-02-20 14:31 ` Konstantin Ananyev
2023-02-20 16:59 ` Tomasz Duszynski
2023-02-20 17:21 ` Konstantin Ananyev
2023-02-20 20:42 ` Tomasz Duszynski
2023-02-21 0:48 ` Konstantin Ananyev
2023-02-27 8:12 ` Tomasz Duszynski
2023-02-28 11:35 ` Konstantin Ananyev [this message]
2023-02-21 12:15 ` Konstantin Ananyev
2023-02-21 2:17 ` Konstantin Ananyev
2023-02-27 9:19 ` [EXT] " Tomasz Duszynski
2023-02-27 20:53 ` Konstantin Ananyev
2023-02-28 8:25 ` Morten Brørup
2023-02-28 12:04 ` Konstantin Ananyev
2023-02-28 13:15 ` Morten Brørup
2023-02-28 16:22 ` Morten Brørup
2023-03-05 16:30 ` Konstantin Ananyev
2023-02-28 9:57 ` Tomasz Duszynski
2023-02-28 11:58 ` Konstantin Ananyev
2023-02-16 17:55 ` [PATCH v11 2/4] pmu: support reading ARM PMU events in runtime Tomasz Duszynski
2023-02-16 17:55 ` [PATCH v11 3/4] pmu: support reading Intel x86_64 " Tomasz Duszynski
2023-02-16 17:55 ` [PATCH v11 4/4] eal: add PMU support to tracing library Tomasz Duszynski
2023-02-16 18:03 ` [PATCH v11 0/4] add support for self monitoring Ruifeng Wang
2023-05-04 8:02 ` David Marchand
2023-07-31 12:33 ` Thomas Monjalon
2023-08-07 8:11 ` [EXT] " Tomasz Duszynski
2023-09-21 8:26 ` David Marchand
2023-01-25 10:33 ` [PATCH 0/2] add platform bus Tomasz Duszynski
2023-01-25 10:33 ` [PATCH 1/2] lib: add helper to read strings from sysfs files Tomasz Duszynski
2023-01-25 10:39 ` Thomas Monjalon
2023-01-25 16:16 ` Tyler Retzlaff
2023-01-26 8:30 ` [EXT] " Tomasz Duszynski
2023-01-26 17:21 ` Tyler Retzlaff
2023-01-26 8:35 ` Tomasz Duszynski
2023-01-25 10:33 ` [PATCH 2/2] bus: add platform bus Tomasz Duszynski
2023-01-25 10:41 ` [PATCH 0/2] " Tomasz Duszynski
2023-02-16 20:56 ` [PATCH v5 0/4] add support for self monitoring Liang Ma
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fdcdeb8db0e04faea8e6b92542bd7306@huawei.com \
--to=konstantin.ananyev@huawei.com \
--cc=dev@dpdk.org \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=tduszynski@marvell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).