diff mbox series

[v3,33/57] perf: Simplify perf_adjust_freq_unthr_context()

Message ID 20230612093539.895253662@infradead.org (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series Scope-based Resource Management | expand

Commit Message

Peter Zijlstra June 12, 2023, 9:07 a.m. UTC 
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Linus Torvalds June 12, 2023, 4:27 p.m. UTC | #1 
This seems to have the same pattern that I *think* it's entirely
broken, ie you are doing that

                guard(perf_pmu_disable)(event->pmu);

inside a loop, and then you are randomly just removing the

               perf_pmu_enable(event->pmu);

at the end of the loop, or when you do a "continue"./

That's not right.

The thing does not not go out of scope when the loop *iterates*.

It only goes out of scope when the loop *ends*.

Big difference as far as cleanup goes.

So you have not "simplified" the unlocking code, you've broken it. Now
it no longer locks and unlocks for each iteration, it tries to re-lock
every time.

Or have I mis-understood something completely?

               Linus
Peter Zijlstra June 12, 2023, 6:44 p.m. UTC | #2 
On Mon, Jun 12, 2023 at 09:27:09AM -0700, Linus Torvalds wrote:

> The thing does not not go out of scope when the loop *iterates*.
> 
> It only goes out of scope when the loop *ends*.

> Or have I mis-understood something completely?

I tried this before I used it and variables inside a for() loop have a
scope of a single iteration.


$ gcc -O2 -o guard guard.c && ./guard
spin_lock
ponies
__raw_spin_lock_irqsave
can haz
raw_spin_unlock_irqrestore
mutex_lock
mutex_unlock
mutex_lock
mutex_unlock
mutex_lock
mutex_unlock
mutex_lock
mutex_unlock
mutex_lock
mutex_unlock
spin_unlock



---
#include <stdio.h>
#include <stdbool.h>

typedef struct {
} raw_spinlock_t;

typedef struct {
} spinlock_t;

typedef struct {
} mutex_t;

void raw_spin_lock(raw_spinlock_t *)
{
	printf("%s\n", __FUNCTION__);
}

void raw_spin_unlock(raw_spinlock_t *)
{
	printf("%s\n", __FUNCTION__);
}

unsigned long __raw_spin_lock_irqsave(raw_spinlock_t *lock)
{
	printf("%s\n", __FUNCTION__);
	return 0;
}
#define raw_spin_lock_irqsave(lock, flags) \
	flags = __raw_spin_lock_irqsave(lock)

void raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
{
	printf("%s\n", __FUNCTION__);
}

void spin_lock(spinlock_t *)
{
	printf("%s\n", __FUNCTION__);
}

void spin_unlock(spinlock_t *)
{
	printf("%s\n", __FUNCTION__);
}


void mutex_lock(mutex_t *)
{
	printf("%s\n", __FUNCTION__);
}

void mutex_unlock(mutex_t *)
{
	printf("%s\n", __FUNCTION__);
}

#define DEFINE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...)			\
typedef struct {								\
	_Type *lock;								\
	__VA_ARGS__								\
} lock_guard_##_type##_t;							\
										\
static inline void lock_guard_##_type##_cleanup(void *_g)	\
{										\
	lock_guard_##_type##_t *_G = _g; \
	if (_G->lock) \
	_Unlock;								\
}										\
										\
static inline lock_guard_##_type##_t lock_guard_##_type##_init(_Type *lock)	\
{										\
	lock_guard_##_type##_t _g = { .lock = lock }, *_G = &_g;		\
	_Lock;									\
	return _g;								\
}

DEFINE_LOCK_GUARD(raw,   raw_spinlock_t,
		  raw_spin_lock(_G->lock),
		  raw_spin_unlock(_G->lock)
		  )

DEFINE_LOCK_GUARD(spin,  spinlock_t,
		  spin_lock(_G->lock),
		  spin_unlock(_G->lock)
		  )

DEFINE_LOCK_GUARD(mutex, mutex_t,
		  mutex_lock(_G->lock),
		  mutex_unlock(_G->lock)
		  )

DEFINE_LOCK_GUARD(raw_irqsave, raw_spinlock_t,
		  raw_spin_lock_irqsave(_G->lock, _G->flags),
		  raw_spin_unlock_irqrestore(_G->lock, _G->flags),
		  unsigned long flags;
		 )

#define __cleanup(func) __attribute__((__cleanup__(func)))

#define lock_guard(_type, _name, _ptr) \
	lock_guard_##_type##_t _name __cleanup(lock_guard_##_type##_cleanup) = \
	lock_guard_##_type##_init(_ptr)

#define lock_scope(_type, _ptr) \
	for (struct { lock_guard_##_type##_t guard ; bool done; } _scope __cleanup(lock_guard_##_type##_cleanup) = \
	     { .guard = lock_guard_##_type##_init(_ptr), .done = false }; \
	     !_scope.done; _scope.done = true)

raw_spinlock_t raw_lock;
spinlock_t lock;
mutex_t mutex;

void main(void)
{
	lock_guard(spin, guard, &lock);
	printf("ponies\n");
	lock_scope(raw_irqsave, &raw_lock) {
		printf("can haz\n");
	}

	for (int i=0; i<5; i++) {
		lock_guard(mutex, foo, &mutex);
		continue;
	}
}
Linus Torvalds June 12, 2023, 6:55 p.m. UTC | #3 
On Mon, Jun 12, 2023 at 11:44 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> I tried this before I used it and variables inside a for() loop have a
> scope of a single iteration.

Whee. Ok, that surprised me. But I guess that I shouldn't have found
it surprising, since the value doesn't survive from one iteration to
the next.

My mental picture of the scope was just different - and apparently wrong.

But thinking about it, it's not just that the value doesn't survive,
it's also that the "continue" will exit the scope in order to go back
to the "for()" loop.

I guess the "goto repeat" ends up being similar, since - as Ian Lance
Taylor said in one of the earlier discussions - that "__cleanup__"
ends up creating an implicit hidden scope for the variable. So a
'goto' to before the variable was declared implicitly exits the scope.

Ugh. I do really hate how subtle that is, though.

So while it might not be the horrible bug I thought it was, I'd
_really_ like us to not do those things just from a sanity angle.

                 Linus
Peter Zijlstra June 12, 2023, 8:05 p.m. UTC | #4 
On Mon, Jun 12, 2023 at 11:55:57AM -0700, Linus Torvalds wrote:

> So while it might not be the horrible bug I thought it was, I'd
> _really_ like us to not do those things just from a sanity angle.

OK, let me go see if there's another way to sanely write those.


Is this less offensive?

restart:
	scoped_guard (rcu) {
		list_for_each_entry_rcu(..) {
			...
			if (err == -EAGAIN)
				goto restart;
		}
	}


That has the explicit scope on the rcu and now the goto explicitly exist
it.

Gonna have to think what to do about the continue ...
Peter Zijlstra June 13, 2023, 12:05 p.m. UTC | #5 
On Mon, Jun 12, 2023 at 11:55:57AM -0700, Linus Torvalds wrote:

> But thinking about it, it's not just that the value doesn't survive,
> it's also that the "continue" will exit the scope in order to go back
> to the "for()" loop.

So if you still feel the continue is a step too far; the alternative
isn't horrible either..

---
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4090,7 +4090,7 @@ perf_adjust_freq_unthr_context(struct pe
 	if (!(ctx->nr_freq || unthrottle))
 		return;
 
-	raw_spin_lock(&ctx->lock);
+	guard(raw_spinlock)(&ctx->lock);
 
 	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
 		if (event->state != PERF_EVENT_STATE_ACTIVE)
@@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct pe
 		if (!event_filter_match(event))
 			continue;
 
-		perf_pmu_disable(event->pmu);
+		guard(perf_pmu_disable)(event->pmu);
 
 		hwc = &event->hw;
 
@@ -4110,34 +4110,29 @@ perf_adjust_freq_unthr_context(struct pe
 			event->pmu->start(event, 0);
 		}
 
-		if (!event->attr.freq || !event->attr.sample_freq)
-			goto next;
+		if (event->attr.freq && event->attr.sample_freq) {
+			/*
+			 * stop the event and update event->count
+			 */
+			event->pmu->stop(event, PERF_EF_UPDATE);
+
+			now = local64_read(&event->count);
+			delta = now - hwc->freq_count_stamp;
+			hwc->freq_count_stamp = now;
+
+			/*
+			 * restart the event
+			 * reload only if value has changed
+			 * we have stopped the event so tell that
+			 * to perf_adjust_period() to avoid stopping it
+			 * twice.
+			 */
+			if (delta > 0)
+				perf_adjust_period(event, period, delta, false);
 
-		/*
-		 * stop the event and update event->count
-		 */
-		event->pmu->stop(event, PERF_EF_UPDATE);
-
-		now = local64_read(&event->count);
-		delta = now - hwc->freq_count_stamp;
-		hwc->freq_count_stamp = now;
-
-		/*
-		 * restart the event
-		 * reload only if value has changed
-		 * we have stopped the event so tell that
-		 * to perf_adjust_period() to avoid stopping it
-		 * twice.
-		 */
-		if (delta > 0)
-			perf_adjust_period(event, period, delta, false);
-
-		event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
-	next:
-		perf_pmu_enable(event->pmu);
+			event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
+		}
 	}
-
-	raw_spin_unlock(&ctx->lock);
 }
 
 /*
diff mbox series

Patch

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4100,7 +4100,7 @@  perf_adjust_freq_unthr_context(struct pe
 	if (!(ctx->nr_freq || unthrottle))
 		return;
 
-	raw_spin_lock(&ctx->lock);
+	guard(raw_spinlock)(&ctx->lock);
 
 	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
 		if (event->state != PERF_EVENT_STATE_ACTIVE)
@@ -4110,7 +4110,7 @@  perf_adjust_freq_unthr_context(struct pe
 		if (!event_filter_match(event))
 			continue;
 
-		perf_pmu_disable(event->pmu);
+		guard(perf_pmu_disable)(event->pmu);
 
 		hwc = &event->hw;
 
@@ -4121,7 +4121,7 @@  perf_adjust_freq_unthr_context(struct pe
 		}
 
 		if (!event->attr.freq || !event->attr.sample_freq)
-			goto next;
+			continue;
 
 		/*
 		 * stop the event and update event->count
@@ -4143,11 +4143,7 @@  perf_adjust_freq_unthr_context(struct pe
 			perf_adjust_period(event, period, delta, false);
 
 		event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
-	next:
-		perf_pmu_enable(event->pmu);
 	}
-
-	raw_spin_unlock(&ctx->lock);
 }
 
 /*