Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1519435
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v3 08/46] perf/x86/intel/cmt: add MONitored Resource (monr) initialization |
| Date | 2016-11-11 00:20 +0100 |
| Message-ID | <sC6Ns-4Ca-13@gated-at.bofh.it> (permalink) |
| References | <sxMtX-5GX-3@gated-at.bofh.it> <sxMtZ-5GX-99@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Sat, 29 Oct 2016, David Carrillo-Cisneros wrote:
> Events use monrs to monitor CMT events. Events that share their
> monitoring target (same thread or cgroup) share a monr.
That wants a proper explanation of the concept of monr before explaining
what is done with them.
> This patch introduces monrs and adds support for monr creation/destruction.
'This patch': We know already that this is a patch. See SubmittingPatches
> An event's associated monr is referenced by event->cmt_monr (introduced
> in previous patch).
>
> monr->mon_events references the first event that uses that monr and events
> that share monr are appended to first event's cmt_list list head.
>
> Hold all pkgd->mutex to modify monr->mon_events and event's data.
Please do not tell what the code does, tell why ad explain the concepts.
> Support for CPU and cgroups is added in future patches in this series.
There is cgroups and CPU and whatever stuff already in this one.
> More details in code's comments.
Please add proper descriptions what this is about instead of describing
what the code does. A changelog should give a reviewer a proper overview
about the functionality it introduces or changes. Sending a reviewer for
comment hunting is not a replacement for that.
> +static void monr_dealloc(struct monr *monr)
> +{
> + kfree(monr);
> +}
Again your ordering of functions is interesting. Please group them together
as they are used. If you need one of them at an earlier point for an error
cleanup or such, then please use forward declarations.
> +static struct monr *monr_alloc(void)
> +{
> + struct monr *monr;
> +
> + lockdep_assert_held(&cmt_mutex);
> +
> + monr = kzalloc(sizeof(*monr), GFP_KERNEL);
> + if (!monr)
> + return ERR_PTR(-ENOMEM);
> +
> + return monr;
> +}
> +
> +static inline struct monr *monr_from_event(struct perf_event *event)
> +{
> + return (struct monr *) READ_ONCE(event->hw.cmt_monr);
That type cast is required because READ_ONCE already returns the type of the
thing you read from, right? So type casting makes more sure of that.
This lacks an explanation why you need a read once at all and it would be
more obvious to put the READ_ONCE explicitely into the code where it is
used and required.
> +}
> +
> +static struct monr *monr_remove_event(struct perf_event *event)
> +{
> + struct monr *monr = monr_from_event(event);
So what is the actual point of the READ_ONCE here?
> +
> + lockdep_assert_held(&cmt_mutex);
> + monr_hrchy_assert_held_mutexes();
> +
> + if (list_empty(&monr->mon_events->hw.cmt_list)) {
> + monr->mon_events = NULL;
> + /* remove from cmt_event_monrs */
> + list_del_init(&monr->entry);
> + } else {
> + if (monr->mon_events == event)
> + monr->mon_events = list_next_entry(event, hw.cmt_list);
> + list_del_init(&event->hw.cmt_list);
> + }
> +
> + WRITE_ONCE(event->hw.cmt_monr, NULL);
And for the WRITE_ONCE here? I'm must be missing something.
event->hw,cmt_monr cannot change here under your feet. So why forcing the
compiler to read it once? The write once is equally pointless. It does not
matter when it is set to NULL.
READ_ONCE/WRITE_ONCE stand out in the code and makes the reader alert so he
starts to look for something which might change under the feet. Just there
is nothing. Annoying.
> +
> + return monr;
> +}
> +
> +static int monr_append_event(struct monr *monr, struct perf_event *event)
> +{
> + lockdep_assert_held(&cmt_mutex);
> + monr_hrchy_assert_held_mutexes();
Again. I really appreciate defensive programming, but with all these
repetated asserts in all these callchains is that code actually making
progress when you have lockdep enabled?
> + if (monr->mon_events) {
> + list_add_tail(&event->hw.cmt_list,
> + &monr->mon_events->hw.cmt_list);
> + } else {
> + monr->mon_events = event;
> + list_add_tail(&monr->entry, &cmt_event_monrs);
> + }
> +
> + WRITE_ONCE(event->hw.cmt_monr, monr);
See above.
> +
> + return 0;
> +}
> +
> +static bool is_cgroup_event(struct perf_event *event)
> +{
> + return false;
> +}
> +
> +static int monr_hrchy_attach_cgroup_event(struct perf_event *event)
> +{
> + return -EPERM;
> +}
> +
> +static int monr_hrchy_attach_cpu_event(struct perf_event *event)
> +{
> + return -EPERM;
> +}
So why is all this here despite the changelog claiming otherwise?
> +
> +static int monr_hrchy_attach_task_event(struct perf_event *event)
> +{
> + struct monr *monr;
> + int err;
> +
> + monr = monr_alloc();
> + if (IS_ERR(monr))
> + return -ENOMEM;
> +
> + err = monr_append_event(monr, event);
> + if (err)
> + monr_dealloc(monr);
> + return err;
> +}
> +
> +/* Insert or create monr in appropriate position in hierarchy. */
> +static int monr_hrchy_attach_event(struct perf_event *event)
> +{
> + int err = 0;
This initialization is pointless.
> +
> + lockdep_assert_held(&cmt_mutex);
> + monr_hrchy_acquire_mutexes();
> +
> + if (!is_cgroup_event(event) &&
> + !(event->attach_state & PERF_ATTACH_TASK)) {
So while you have a faible for wrapping things which should be in the code,
here and else where you have these written out checks over and over.
> + err = monr_hrchy_attach_cpu_event(event);
> + goto exit;
> + }
> + if (is_cgroup_event(event)) {
> + err = monr_hrchy_attach_cgroup_event(event);
> + goto exit;
> + }
> + err = monr_hrchy_attach_task_event(event);
Can we please avoid these gotos?
if (is_cpu_event(event))
err = attach_cpu(event);
else if (is_cgroup_event(event))
err = attach_cgroup(event);
else
err = attach_task(event);
would be compact and readable code without gotos. Hmm?
> +exit:
> + monr_hrchy_release_mutexes();
> +
> + return err;
> +}
> +
> +/**
> + * __match_event() - Determine if @a and @b should share a rmid.
If you use kernel doc comments, then you have to fill out the argument
descriptors as well.
> + */
> +static bool __match_event(struct perf_event *a, struct perf_event *b)
Why underscores? We use underscores when we have something like this:
__bla(...)
{
do stuff ....
}
bla(...)
{
lock();
__bla(...);
unlock();
}
But not for standalone functions which have no particular counterpart which
does extra work like serialization.
> +{
> + /* Cgroup/non-task per-cpu and task events don't mix */
> + if ((a->attach_state & PERF_ATTACH_TASK) !=
> + (b->attach_state & PERF_ATTACH_TASK))
> + return false;
So this would become a simple one liner with a proper wrapper as well
if (is_task_event(a) != is_task_event(b))
return false;
> +
> +#ifdef CONFIG_CGROUP_PERF
> + if (a->cgrp != b->cgrp)
> + return false;
> +#endif
> +
> + /* If not task event, it's a a cgroup or a non-task cpu event. */
> + if (!(b->attach_state & PERF_ATTACH_TASK))
> + return true;
> +
> + /* Events that target same task are placed into the same group. */
> + if (a->hw.target == b->hw.target)
> + return true;
> +
> + /* Are we a inherited event? */
> + if (b->parent == a)
> + return true;
> +
> + return false;
> +}
> +
> static struct pmu intel_cmt_pmu;
The placement of this in the middle of unrelated code does what? It's
really annoying having to search for stuff at random places.
Please be more careful about placement.
>
> +/* Try to find a monr with same target, otherwise create new one. */
> +static int mon_group_setup_event(struct perf_event *event)
> +{
> + struct monr *monr;
> + int err;
> +
> + lockdep_assert_held(&cmt_mutex);
> +
> + list_for_each_entry(monr, &cmt_event_monrs, entry) {
> + if (!__match_event(monr->mon_events, event))
> + continue;
> + monr_hrchy_acquire_mutexes();
> + err = monr_append_event(monr, event);
> + monr_hrchy_release_mutexes();
> + return err;
> + }
> + /*
> + * Since no match was found, create a new monr and set this
> + * event as head of a mon_group. All events in this group
> + * will share the monr.
> + */
> + return monr_hrchy_attach_event(event);
> +}
> +
> static void intel_cmt_event_read(struct perf_event *event)
> {
> }
> @@ -68,6 +272,20 @@ static int intel_cmt_event_add(struct perf_event *event, int mode)
> return 0;
> }
>
> +static void intel_cmt_event_destroy(struct perf_event *event)
So I was looking at the fully applied series and could not find this
function anymore because it was renamed at some random point. Can you
please avoid this for new code? If you rework something existing it might
be necessary, but for new code it's just a pointless exercise.
> +{
> + struct monr *monr;
> +
> + mutex_lock(&cmt_mutex);
> + monr_hrchy_acquire_mutexes();
> +
> + /* monr is dettached from event. */
Please comment things which are not obvious. I already know that the event
is destroyed and therefore the monr has to be detached. Comments should
explain why and not what. 'What' we can see from the code and if that has
well chosen names, then it's self explanatory. But the 'Why' of something
complex does not spring into the readers brain right away. That's what
comments are for.
> + monr = monr_remove_event(event);
Now, whats entirely non obvious is the assignement of the return value with
no further check or processing. I know you do that in some follow up patch,
but for now it's just adding a compiler warning.
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v3 00/46] Cache Monitoring Technology (aka CQM) David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 25/46] perf/x86/intel/cmt: add monr->flags and CMT_MONR_ZOMBIE David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 20/46] perf/core: add pmu::event_terminate David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 22/46] perf/x86/intel/cmt: sync cgroups and intel_cmt device start/stop David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 36/46] perf/core: add perf_event cgroup hooks for subsystem attributes David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 23/46] perf/core: hooks to add architecture specific features in perf_cgroup David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 17/46] perf/x86/intel/cmt: add uflag CMT_UF_NOLAZY_RMID David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 22:30 +0100
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks David Carrillo-Cisneros <davidcc@google.com> - 2016-11-11 03:30 +0100
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks Peter Zijlstra <peterz@infradead.org> - 2016-11-11 08:30 +0100
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks Ingo Molnar <mingo@kernel.org> - 2016-11-11 08:40 +0100
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks Thomas Gleixner <tglx@linutronix.de> - 2016-11-11 10:50 +0100
Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks David Carrillo-Cisneros <davidcc@google.com> - 2016-11-11 18:30 +0100
[PATCH v3 09/46] perf/x86/intel/cmt: add basic monr hierarchy David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 26/46] sched: introduce the finish_arch_pre_lock_switch() scheduler hook David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 40/46] perf/x86/intel/cmt: add rotation scheduled work David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 27/46] perf/x86/intel: add pqr cache flags and intel_pqr_ctx_switch David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 37/46] perf/x86/intel/cmt: add cont_monitoring to perf cgroup David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 35/46] perf/x86/intel/cmt: use PERF_EV_CAP_CGROUP_NO_RECURSION in intel_cmt David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 16/46] perf/x86/intel/cmt: set sched rmid and complete pmu start/stop/add/del David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 41/46] perf/x86/intel/cmt: add rotation minimum progress SLO David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 02/46] perf/x86/intel: rename CQM cpufeatures to CMT David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 45/46] perf/stat: fix bug in handling events in error state David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 28/46] perf,perf/x86,perf/powerpc,perf/arm,perf/*: add int error return to pmu::read David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 34/46] perf/core: introduce PERF_EV_CAP_CGROUP_NO_RECURSION David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 15/46] perf/x86/intel: encapsulate rmid and closid updates in pqr cache David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 46/46] perf/stat: revamp read error handling, snapshot and per_pkg events David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 24/46] perf/x86/intel/cmt: add perf_cgroup_arch_css_{online,offline} David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 10/46] perf/x86/intel/cmt: add Package MONitored Resource (pmonr) initialization David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 04/46] perf/x86/intel/cmt: add device initialization and CPU hotplug support David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
Re: [PATCH v3 04/46] perf/x86/intel/cmt: add device initialization and CPU hotplug support Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 16:30 +0100
[PATCH v3 29/46] perf/x86/intel/cmt: add error handling to intel_cmt_event_read David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 43/46] perf/x86/intel/cmt: add CMT_UF_NOSTEAL_RMID flag David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 07/46] perf/core: add RDT Monitoring attributes to struct hw_perf_event David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 18/46] perf/core: add arch_info field to struct perf_cgroup David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 38/46] perf/x86/intel/cmt: introduce read SLOs for rotation David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 42/46] perf/x86/intel/cmt: add rmid stealing David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 30/46] perf/x86/intel/cmt: add asynchronous read for task events David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 44/46] perf/x86/intel/cmt: add debugfs intel_cmt directory David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 32/46] perf/core: Add PERF_EV_CAP_READ_ANY_{CPU_,}PKG flags David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 12/46] perf/x86/intel/cmt: add per-package rmid pools David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 39/46] perf/x86/intel/cmt: add max_recycle_threshold sysfs attribute David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 33/46] perf/x86/intel/cmt: use PERF_EV_CAP_READ_{,CPU_}PKG flags in Intel cmt David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 13/46] perf/x86/intel/cmt: add pmonr's Off and Unused states David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 14/46] perf/x86/intel/cmt: add Active and Dep_{Idle, Dirty} states David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 21/46] perf/x86/intel/cmt: use newly introduced event_terminate David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 11/46] perf/x86/intel/cmt: add cmt_user_flags (uflags) to monr David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
[PATCH v3 08/46] perf/x86/intel/cmt: add MONitored Resource (monr) initialization David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:50 +0200
Re: [PATCH v3 08/46] perf/x86/intel/cmt: add MONitored Resource (monr) initialization Thomas Gleixner <tglx@linutronix.de> - 2016-11-11 00:20 +0100
[PATCH v3 06/46] perf/x86/intel/cmt: add intel_cmt pmu David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:00 +0100
Re: [PATCH v3 06/46] perf/x86/intel/cmt: add intel_cmt pmu Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 22:40 +0100
[PATCH v3 03/46] x86/intel: add CONFIG_INTEL_RDT_M configuration flag David Carrillo-Cisneros <davidcc@google.com> - 2016-10-30 02:00 +0100
csiph-web