Re: [PATCH] x86: UV: raw_spinlock conversion
On Tue, 2018-05-22 at 11:46 +0200, Mike Galbraith wrote: > On Tue, 2018-05-22 at 11:14 +0200, Sebastian Andrzej Siewior wrote: > > > If you suggest that I > > should stop caring about UV than I do so. Please post a patch that adds > > a dependency to UV on PREEMPT so that part of the architecture is > > documented. > > Will do. On second thought, no I won't. It's either already known, or it should be, making any such submission smell funny. -Mike
Re: [PATCH] x86: UV: raw_spinlock conversion
On Tue, 2018-05-22 at 11:14 +0200, Sebastian Andrzej Siewior wrote: > On 2018-05-22 10:24:22 [+0200], Mike Galbraith wrote: > > > If I were in your shoes, I think I'd just stop caring about UV until a > > real user appears. AFAIK, I'm the only guy who ever ran RT on UV, and > > I only did so because SUSE asked me to look into it.. years ago now. > > Okay. The problem I have with this patch is that it remains RT only > while the problem it addresses is not RT-only and PREEMPT kernels are > very much affected. Ah, but when RT gets merged (someday... maybe), that patch will apply, and instantly make all.. zero.. UV-RT users happy campers :) > The thing is that *you* are my only UV user :) Crash-test-dummies don't really qualify as users :) > If you suggest that I > should stop caring about UV than I do so. Please post a patch that adds > a dependency to UV on PREEMPT so that part of the architecture is > documented. Will do. -Mike
Re: [PATCH] x86: UV: raw_spinlock conversion
On 2018-05-22 10:24:22 [+0200], Mike Galbraith wrote: > On Tue, 2018-05-22 at 08:50 +0200, Sebastian Andrzej Siewior wrote: > > > > Regarding the preempt_disable() in the original patch in uv_read_rtc(): > > This looks essential for PREEMPT configs. Is it possible to get this > > tested by someone or else get rid of the UV code? It looks broken for > > "uv_get_min_hub_revision_id() != 1". > > I suspect SGI cares not one whit about PREEMPT. so it is broken then. I leave it to the x86 maintainers but on the very least it should depend on !PREEMPT (if not server). > > Why does PREEMPT_RT require migrate_disable() but PREEMPT only is fine > > as-is? This does not look right. > > UV is not ok with a PREEMPT config, it's just that for RT it's dirt > simple to shut it up, whereas for PREEMPT, preempt_disable() across > uv_bau_init() doesn't cut it due to allocations, and whatever else I > would have met before ending the whack-a-mole game. > > If I were in your shoes, I think I'd just stop caring about UV until a > real user appears. AFAIK, I'm the only guy who ever ran RT on UV, and > I only did so because SUSE asked me to look into it.. years ago now. Okay. The problem I have with this patch is that it remains RT only while the problem it addresses is not RT-only and PREEMPT kernels are very much affected. The thing is that *you* are my only UV user :) If you suggest that I should stop caring about UV than I do so. Please post a patch that adds a dependency to UV on PREEMPT so that part of the architecture is documented. > -Mike Sebastian
Re: [PATCH] x86: UV: raw_spinlock conversion
On Tue, 2018-05-22 at 08:50 +0200, Sebastian Andrzej Siewior wrote: > > Regarding the preempt_disable() in the original patch in uv_read_rtc(): > This looks essential for PREEMPT configs. Is it possible to get this > tested by someone or else get rid of the UV code? It looks broken for > "uv_get_min_hub_revision_id() != 1". I suspect SGI cares not one whit about PREEMPT. > Why does PREEMPT_RT require migrate_disable() but PREEMPT only is fine > as-is? This does not look right. UV is not ok with a PREEMPT config, it's just that for RT it's dirt simple to shut it up, whereas for PREEMPT, preempt_disable() across uv_bau_init() doesn't cut it due to allocations, and whatever else I would have met before ending the whack-a-mole game. If I were in your shoes, I think I'd just stop caring about UV until a real user appears. AFAIK, I'm the only guy who ever ran RT on UV, and I only did so because SUSE asked me to look into it.. years ago now. -Mike
Re: [PATCH] x86: UV: raw_spinlock conversion
On 2018-05-19 16:09:56 [+0200], Mike Galbraith wrote:
> None of that patch is needed for a UV3000, but the below is. It's
> likely still valid for now ancient UV boxen, but the UV100 the patch
> was originally written for (2011/2.6.33-rt) has apparently wandered off
> to become a beer keg or something meanwhile, so I can't test.
so the old patch can go. Noted.
Regarding the preempt_disable() in the original patch in uv_read_rtc():
This looks essential for PREEMPT configs. Is it possible to get this
tested by someone or else get rid of the UV code? It looks broken for
"uv_get_min_hub_revision_id() != 1".
> UV: Fix uv_bau_init() check_preemption_disabled() gripeage
>
> [2.851947] BUG: using smp_processor_id() in preemptible [] code:
> swapper/0/1
> [2.851951] caller is uv_bau_init+0x28/0xb62
> [2.851954] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 4.17.0-rc5-0.g3e3e37b-rt_debug
> [2.851956] Hardware name: SGI UV3000/UV3000, BIOS SGI UV 3000 series BIOS
> 01/15/2015
> [2.851957] Call Trace:
> [2.851964] dump_stack+0x85/0xcb
> [2.851969] check_preemption_disabled+0x10c/0x120
> [2.851972] ? init_per_cpu+0x88c/0x88c
> [2.851974] uv_bau_init+0x28/0xb62
> [2.851979] ? lapic_cal_handler+0xbb/0xbb
> [2.851982] ? rt_mutex_unlock+0x35/0x50
> [2.851985] ? init_per_cpu+0x88c/0x88c
> [2.851988] ? set_debug_rodata+0x11/0x11
> [2.851991] do_one_initcall+0x46/0x249
> [2.851995] kernel_init_freeable+0x207/0x29c
> [2.851999] ? rest_init+0xd0/0xd0
> [2.852000] kernel_init+0xa/0x110
> [2.852000] ret_from_fork+0x3a/0x50
>
> (gdb) list *uv_bau_init+0x28
> 0x824a4d96 is in uv_bau_init (./arch/x86/include/asm/uv/uv_hub.h:212).
> 207 return (struct uv_hub_info_s *)__uv_hub_info_list[node];
> 208 }
> 209
> 210 static inline struct uv_hub_info_s *_uv_hub_info(void)
> 211 {
> 212 return (struct uv_hub_info_s *)uv_cpu_info->p_uv_hub_info;
> 213 }
> 214 #define uv_hub_info _uv_hub_info()
> 215
> 216 static inline struct uv_hub_info_s *uv_cpu_hub_info(int cpu)
> (gdb)
>
> arch/x86/include/asm/uv/uv_hub.h:
> 197 #define uv_cpu_info this_cpu_ptr(&__uv_cpu_info)
>
> This and other substitutions make uv_bau_init() annoying for a PREEMPT
> kernel, but PREEMPT_RT can silence the lot with one migrate_disable().
Why does PREEMPT_RT require migrate_disable() but PREEMPT only is fine
as-is? This does not look right.
> Signed-off-by: Mike Galbraith
> ---
> arch/x86/platform/uv/tlb_uv.c |5 +
> 1 file changed, 5 insertions(+)
>
> --- a/arch/x86/platform/uv/tlb_uv.c
> +++ b/arch/x86/platform/uv/tlb_uv.c
> @@ -2213,6 +2213,8 @@ static int __init uv_bau_init(void)
> if (!is_uv_system())
> return 0;
>
> + migrate_disable();
> +
> if (is_uv4_hub())
> ops = uv4_bau_ops;
> else if (is_uv3_hub())
> @@ -2269,6 +2271,8 @@ static int __init uv_bau_init(void)
> }
> }
>
> + migrate_enable();
> +
> return 0;
>
> err_bau_disable:
> @@ -2276,6 +2280,7 @@ static int __init uv_bau_init(void)
> for_each_possible_cpu(cur_cpu)
> free_cpumask_var(per_cpu(uv_flush_tlb_mask, cur_cpu));
>
> + migrate_enable();
> set_bau_off();
> nobau_perm = 1;
>
Sebastian
Re: [PATCH] x86: UV: raw_spinlock conversion
On Mon, 2018-05-07 at 09:39 +0200, Sebastian Andrzej Siewior wrote:
> On 2018-05-06 12:59:19 [+0200], Mike Galbraith wrote:
> > On Sun, 2018-05-06 at 12:26 +0200, Thomas Gleixner wrote:
> > > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
> > >
> > > > From: Mike Galbraith
> > > >
> > > > Shrug. Lots of hobbyists have a beast in their basement, right?
> > >
> > > This hardly qualifies as a proper changelog ...
> >
> > Hm, that wasn't intended to be a changelog.
> >
> > This patch may not be current either, I haven't tested RT on a UV box
> > in quite some time.
>
> That last hunk looks like something that would be required even for !RT.
> Would you mind to check that patch and write a changelog? If it doesn't
> work for RT there is no need to carry this in -RT.
None of that patch is needed for a UV3000, but the below is. It's
likely still valid for now ancient UV boxen, but the UV100 the patch
was originally written for (2011/2.6.33-rt) has apparently wandered off
to become a beer keg or something meanwhile, so I can't test.
UV: Fix uv_bau_init() check_preemption_disabled() gripeage
[2.851947] BUG: using smp_processor_id() in preemptible [] code:
swapper/0/1
[2.851951] caller is uv_bau_init+0x28/0xb62
[2.851954] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.17.0-rc5-0.g3e3e37b-rt_debug
[2.851956] Hardware name: SGI UV3000/UV3000, BIOS SGI UV 3000 series BIOS
01/15/2015
[2.851957] Call Trace:
[2.851964] dump_stack+0x85/0xcb
[2.851969] check_preemption_disabled+0x10c/0x120
[2.851972] ? init_per_cpu+0x88c/0x88c
[2.851974] uv_bau_init+0x28/0xb62
[2.851979] ? lapic_cal_handler+0xbb/0xbb
[2.851982] ? rt_mutex_unlock+0x35/0x50
[2.851985] ? init_per_cpu+0x88c/0x88c
[2.851988] ? set_debug_rodata+0x11/0x11
[2.851991] do_one_initcall+0x46/0x249
[2.851995] kernel_init_freeable+0x207/0x29c
[2.851999] ? rest_init+0xd0/0xd0
[2.852000] kernel_init+0xa/0x110
[2.852000] ret_from_fork+0x3a/0x50
(gdb) list *uv_bau_init+0x28
0x824a4d96 is in uv_bau_init (./arch/x86/include/asm/uv/uv_hub.h:212).
207 return (struct uv_hub_info_s *)__uv_hub_info_list[node];
208 }
209
210 static inline struct uv_hub_info_s *_uv_hub_info(void)
211 {
212 return (struct uv_hub_info_s *)uv_cpu_info->p_uv_hub_info;
213 }
214 #define uv_hub_info _uv_hub_info()
215
216 static inline struct uv_hub_info_s *uv_cpu_hub_info(int cpu)
(gdb)
arch/x86/include/asm/uv/uv_hub.h:
197 #define uv_cpu_info this_cpu_ptr(&__uv_cpu_info)
This and other substitutions make uv_bau_init() annoying for a PREEMPT
kernel, but PREEMPT_RT can silence the lot with one migrate_disable().
Signed-off-by: Mike Galbraith
---
arch/x86/platform/uv/tlb_uv.c |5 +
1 file changed, 5 insertions(+)
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -2213,6 +2213,8 @@ static int __init uv_bau_init(void)
if (!is_uv_system())
return 0;
+ migrate_disable();
+
if (is_uv4_hub())
ops = uv4_bau_ops;
else if (is_uv3_hub())
@@ -2269,6 +2271,8 @@ static int __init uv_bau_init(void)
}
}
+ migrate_enable();
+
return 0;
err_bau_disable:
@@ -2276,6 +2280,7 @@ static int __init uv_bau_init(void)
for_each_possible_cpu(cur_cpu)
free_cpumask_var(per_cpu(uv_flush_tlb_mask, cur_cpu));
+ migrate_enable();
set_bau_off();
nobau_perm = 1;
Re: [PATCH] x86: UV: raw_spinlock conversion
On Mon, 2018-05-07 at 09:39 +0200, Sebastian Andrzej Siewior wrote: > On 2018-05-06 12:59:19 [+0200], Mike Galbraith wrote: > > On Sun, 2018-05-06 at 12:26 +0200, Thomas Gleixner wrote: > > > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote: > > > > > > > From: Mike Galbraith > > > > > > > > Shrug. Lots of hobbyists have a beast in their basement, right? > > > > > > This hardly qualifies as a proper changelog ... > > > > Hm, that wasn't intended to be a changelog. > > > > This patch may not be current either, I haven't tested RT on a UV box > > in quite some time. > > That last hunk looks like something that would be required even for !RT. > Would you mind to check that patch and write a changelog? If it doesn't > work for RT there is no need to carry this in -RT. Yeah, I'll try to reserve a box. -Mike
Re: [PATCH] x86: UV: raw_spinlock conversion
On 2018-05-06 12:59:19 [+0200], Mike Galbraith wrote: > On Sun, 2018-05-06 at 12:26 +0200, Thomas Gleixner wrote: > > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote: > > > > > From: Mike Galbraith > > > > > > Shrug. Lots of hobbyists have a beast in their basement, right? > > > > This hardly qualifies as a proper changelog ... > > Hm, that wasn't intended to be a changelog. > > This patch may not be current either, I haven't tested RT on a UV box > in quite some time. That last hunk looks like something that would be required even for !RT. Would you mind to check that patch and write a changelog? If it doesn't work for RT there is no need to carry this in -RT. > -Mike Sebastian
Re: [PATCH] x86: UV: raw_spinlock conversion
On Sun, 2018-05-06 at 12:26 +0200, Thomas Gleixner wrote: > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote: > > > From: Mike Galbraith > > > > Shrug. Lots of hobbyists have a beast in their basement, right? > > This hardly qualifies as a proper changelog ... Hm, that wasn't intended to be a changelog. This patch may not be current either, I haven't tested RT on a UV box in quite some time. -Mike
Re: [PATCH] x86: UV: raw_spinlock conversion
On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
> From: Mike Galbraith
>
> Shrug. Lots of hobbyists have a beast in their basement, right?
This hardly qualifies as a proper changelog ...
> }
> @@ -299,13 +299,17 @@ static int uv_rtc_unset_timer(int cpu, i
> static u64 uv_read_rtc(struct clocksource *cs)
> {
> unsigned long offset;
> + u64 cycles;
>
> + preempt_disable();
> if (uv_get_min_hub_revision_id() == 1)
> offset = 0;
> else
> offset = (uv_blade_processor_id() * L1_CACHE_BYTES) % PAGE_SIZE;
>
> - return (u64)uv_read_local_mmr(UVH_RTC | offset);
> + cycles = (u64)uv_read_local_mmr(UVH_RTC | offset);
> + preempt_enable();
> + return cycles;
And how exaclty is this hunk related?
Thanks,
tglx
[PATCH] x86: UV: raw_spinlock conversion
From: Mike Galbraith
Shrug. Lots of hobbyists have a beast in their basement, right?
Cc: x...@kernel.org
Signed-off-by: Mike Galbraith
Signed-off-by: Sebastian Andrzej Siewior
---
arch/x86/include/asm/uv/uv_bau.h | 14 +++---
arch/x86/platform/uv/tlb_uv.c| 26 +-
arch/x86/platform/uv/uv_time.c | 20
3 files changed, 32 insertions(+), 28 deletions(-)
--- a/arch/x86/include/asm/uv/uv_bau.h
+++ b/arch/x86/include/asm/uv/uv_bau.h
@@ -643,9 +643,9 @@ struct bau_control {
cycles_tsend_message;
cycles_tperiod_end;
cycles_tperiod_time;
- spinlock_t uvhub_lock;
- spinlock_t queue_lock;
- spinlock_t disable_lock;
+ raw_spinlock_t uvhub_lock;
+ raw_spinlock_t queue_lock;
+ raw_spinlock_t disable_lock;
/* tunables */
int max_concurr;
int max_concurr_const;
@@ -847,15 +847,15 @@ static inline int atom_asr(short i, stru
* to be lowered below the current 'v'. atomic_add_unless can only stop
* on equal.
*/
-static inline int atomic_inc_unless_ge(spinlock_t *lock, atomic_t *v, int u)
+static inline int atomic_inc_unless_ge(raw_spinlock_t *lock, atomic_t *v, int
u)
{
- spin_lock(lock);
+ raw_spin_lock(lock);
if (atomic_read(v) >= u) {
- spin_unlock(lock);
+ raw_spin_unlock(lock);
return 0;
}
atomic_inc(v);
- spin_unlock(lock);
+ raw_spin_unlock(lock);
return 1;
}
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -740,9 +740,9 @@ static void destination_plugged(struct b
quiesce_local_uvhub(hmaster);
- spin_lock(&hmaster->queue_lock);
+ raw_spin_lock(&hmaster->queue_lock);
reset_with_ipi(&bau_desc->distribution, bcp);
- spin_unlock(&hmaster->queue_lock);
+ raw_spin_unlock(&hmaster->queue_lock);
end_uvhub_quiesce(hmaster);
@@ -762,9 +762,9 @@ static void destination_timeout(struct b
quiesce_local_uvhub(hmaster);
- spin_lock(&hmaster->queue_lock);
+ raw_spin_lock(&hmaster->queue_lock);
reset_with_ipi(&bau_desc->distribution, bcp);
- spin_unlock(&hmaster->queue_lock);
+ raw_spin_unlock(&hmaster->queue_lock);
end_uvhub_quiesce(hmaster);
@@ -785,7 +785,7 @@ static void disable_for_period(struct ba
cycles_t tm1;
hmaster = bcp->uvhub_master;
- spin_lock(&hmaster->disable_lock);
+ raw_spin_lock(&hmaster->disable_lock);
if (!bcp->baudisabled) {
stat->s_bau_disabled++;
tm1 = get_cycles();
@@ -798,7 +798,7 @@ static void disable_for_period(struct ba
}
}
}
- spin_unlock(&hmaster->disable_lock);
+ raw_spin_unlock(&hmaster->disable_lock);
}
static void count_max_concurr(int stat, struct bau_control *bcp,
@@ -861,7 +861,7 @@ static void record_send_stats(cycles_t t
*/
static void uv1_throttle(struct bau_control *hmaster, struct ptc_stats *stat)
{
- spinlock_t *lock = &hmaster->uvhub_lock;
+ raw_spinlock_t *lock = &hmaster->uvhub_lock;
atomic_t *v;
v = &hmaster->active_descriptor_count;
@@ -995,7 +995,7 @@ static int check_enable(struct bau_contr
struct bau_control *hmaster;
hmaster = bcp->uvhub_master;
- spin_lock(&hmaster->disable_lock);
+ raw_spin_lock(&hmaster->disable_lock);
if (bcp->baudisabled && (get_cycles() >= bcp->set_bau_on_time)) {
stat->s_bau_reenabled++;
for_each_present_cpu(tcpu) {
@@ -1007,10 +1007,10 @@ static int check_enable(struct bau_contr
tbcp->period_giveups = 0;
}
}
- spin_unlock(&hmaster->disable_lock);
+ raw_spin_unlock(&hmaster->disable_lock);
return 0;
}
- spin_unlock(&hmaster->disable_lock);
+ raw_spin_unlock(&hmaster->disable_lock);
return -1;
}
@@ -1941,9 +1941,9 @@ static void __init init_per_cpu_tunables
bcp->cong_reps = congested_reps;
bcp->disabled_period= sec_2_cycles(disabled_period);
bcp->giveup_limit = giveup_limit;
- spin_lock_init(&bcp->queue_lock);
- spin_lock_init(&bcp->uvhub_lock);
- spin_lock_init(&bcp->disable_lock);
+ raw_spin_lock_init(&bcp->queue_lock);
+ raw_spin_lock_init(&bcp->uvhub_lock);
+ raw_spin_lock_init(&bcp->disable_lock);
}
}
--- a/arch/x86/platform/uv/uv_time.c
