From: Cliff Wickman <[email protected]>
Calling smp_processor_id() from within a preemptable region will issue
a warning if DEBUG_PREEMPT is set.
Diffed against 3.0.0-rc3
Signed-off-by: Cliff Wickman <[email protected]>
---
arch/x86/platform/uv/tlb_uv.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux/arch/x86/platform/uv/tlb_uv.c
===================================================================
--- linux.orig/arch/x86/platform/uv/tlb_uv.c
+++ linux/arch/x86/platform/uv/tlb_uv.c
@@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
instr[count] = '\0';
+ preempt_disable(); /* avoid DEBUG_PREEMPT warning */
bcp = &per_cpu(bau_control, smp_processor_id());
+ preempt_enable_no_resched();
ret = parse_tunables_write(bcp, instr, count);
if (ret)
On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> From: Cliff Wickman <[email protected]>
>
> Calling smp_processor_id() from within a preemptable region will issue
> a warning if DEBUG_PREEMPT is set.
>
> Diffed against 3.0.0-rc3
>
> Signed-off-by: Cliff Wickman <[email protected]>
> ---
> ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> ?1 file changed, 2 insertions(+)
>
> Index: linux/arch/x86/platform/uv/tlb_uv.c
> ===================================================================
> --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> +++ linux/arch/x86/platform/uv/tlb_uv.c
> @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
>
> ? ? ? ?instr[count] = '\0';
>
> + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
something more meaningful. It's a BUG, if smp_processor_id() is called
within preemptible context. So, we don't want to hit that BUG.
> ? ? ? ?bcp = &per_cpu(bau_control, smp_processor_id());
> + ? ? ? preempt_enable_no_resched();
>
> ? ? ? ?ret = parse_tunables_write(bcp, instr, count);
> ? ? ? ?if (ret)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> > From: Cliff Wickman <[email protected]>
> >
> > Calling smp_processor_id() from within a preemptable region will issue
> > a warning if DEBUG_PREEMPT is set.
> >
> > Diffed against 3.0.0-rc3
> >
> > Signed-off-by: Cliff Wickman <[email protected]>
> > ---
> > ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> > ?1 file changed, 2 insertions(+)
> >
> > Index: linux/arch/x86/platform/uv/tlb_uv.c
> > ===================================================================
> > --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> > +++ linux/arch/x86/platform/uv/tlb_uv.c
> > @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
> >
> > ? ? ? ?instr[count] = '\0';
> >
> > + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
>
> I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
> something more meaningful. It's a BUG, if smp_processor_id() is called
> within preemptible context. So, we don't want to hit that BUG.
I agree that calling smp_processor_id() within a preemptible context is
going to produce unpredictable results. In this particular case we just
need a valid cpu number so that we can find a per-cpu structure.
That structure contains a reasonable (sanity-checking) limit to the value
of the tunable that is being written.
It is possible that the found per-cpu structure could differ from cpu to
cpu. But if this tunable is thus caused to be set too high, a 'throttling'
upper bound will not be enforced. No other harm is done.
But yes, I should have noted that an ugly DEBUG_PREEMPT warning will
be the worst effect.
-Cliff
>
> > ? ? ? ?bcp = &per_cpu(bau_control, smp_processor_id());
> > + ? ? ? preempt_enable_no_resched();
> >
> > ? ? ? ?ret = parse_tunables_write(bcp, instr, count);
> > ? ? ? ?if (ret)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at ?http://www.tux.org/lkml/
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Cliff Wickman
SGI
[email protected]
(651) 683-3824
* Cliff Wickman <[email protected]> wrote:
> On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> > On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> > > From: Cliff Wickman <[email protected]>
> > >
> > > Calling smp_processor_id() from within a preemptable region will issue
> > > a warning if DEBUG_PREEMPT is set.
> > >
> > > Diffed against 3.0.0-rc3
> > >
> > > Signed-off-by: Cliff Wickman <[email protected]>
> > > ---
> > > ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> > > ?1 file changed, 2 insertions(+)
> > >
> > > Index: linux/arch/x86/platform/uv/tlb_uv.c
> > > ===================================================================
> > > --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> > > +++ linux/arch/x86/platform/uv/tlb_uv.c
> > > @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
> > >
> > > ? ? ? ?instr[count] = '\0';
> > >
> > > + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
> >
> > I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
> > something more meaningful. It's a BUG, if smp_processor_id() is called
> > within preemptible context. So, we don't want to hit that BUG.
>
> I agree that calling smp_processor_id() within a preemptible context is
> going to produce unpredictable results. In this particular case we just
> need a valid cpu number so that we can find a per-cpu structure.
> That structure contains a reasonable (sanity-checking) limit to the value
> of the tunable that is being written.
So what happens if the code gets preempted away and this CPU is
hotplugged away? You'll reference a CPU ID that does not exist
anymore.
Thanks,
Ingo
Hi Ingo,
On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
>
> * Cliff Wickman <[email protected]> wrote:
>
> > On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> > > On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> > > > From: Cliff Wickman <[email protected]>
> > > >
> > > > Calling smp_processor_id() from within a preemptable region will issue
> > > > a warning if DEBUG_PREEMPT is set.
> > > >
> > > > Diffed against 3.0.0-rc3
> > > >
> > > > Signed-off-by: Cliff Wickman <[email protected]>
> > > > ---
> > > > ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> > > > ?1 file changed, 2 insertions(+)
> > > >
> > > > Index: linux/arch/x86/platform/uv/tlb_uv.c
> > > > ===================================================================
> > > > --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> > > > +++ linux/arch/x86/platform/uv/tlb_uv.c
> > > > @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
> > > >
> > > > ? ? ? ?instr[count] = '\0';
> > > >
> > > > + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
> > >
> > > I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
> > > something more meaningful. It's a BUG, if smp_processor_id() is called
> > > within preemptible context. So, we don't want to hit that BUG.
> >
> > I agree that calling smp_processor_id() within a preemptible context is
> > going to produce unpredictable results. In this particular case we just
> > need a valid cpu number so that we can find a per-cpu structure.
> > That structure contains a reasonable (sanity-checking) limit to the value
> > of the tunable that is being written.
>
> So what happens if the code gets preempted away and this CPU is
> hotplugged away? You'll reference a CPU ID that does not exist
> anymore.
You're right of course. But we don't support CPU hotplug on the UV hardware.
There are enhancements needed in both the BIOS and Linux (BAU and GRU among
them). They are on our work queue.
-Cliff
--
Cliff Wickman
SGI
[email protected]
(651) 683-3824
* Cliff Wickman <[email protected]> wrote:
> Hi Ingo,
>
> On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
> >
> > * Cliff Wickman <[email protected]> wrote:
> >
> > > On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> > > > On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> > > > > From: Cliff Wickman <[email protected]>
> > > > >
> > > > > Calling smp_processor_id() from within a preemptable region will issue
> > > > > a warning if DEBUG_PREEMPT is set.
> > > > >
> > > > > Diffed against 3.0.0-rc3
> > > > >
> > > > > Signed-off-by: Cliff Wickman <[email protected]>
> > > > > ---
> > > > > ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> > > > > ?1 file changed, 2 insertions(+)
> > > > >
> > > > > Index: linux/arch/x86/platform/uv/tlb_uv.c
> > > > > ===================================================================
> > > > > --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> > > > > +++ linux/arch/x86/platform/uv/tlb_uv.c
> > > > > @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
> > > > >
> > > > > ? ? ? ?instr[count] = '\0';
> > > > >
> > > > > + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
> > > >
> > > > I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
> > > > something more meaningful. It's a BUG, if smp_processor_id() is called
> > > > within preemptible context. So, we don't want to hit that BUG.
> > >
> > > I agree that calling smp_processor_id() within a preemptible context is
> > > going to produce unpredictable results. In this particular case we just
> > > need a valid cpu number so that we can find a per-cpu structure.
> > > That structure contains a reasonable (sanity-checking) limit to the value
> > > of the tunable that is being written.
> >
> > So what happens if the code gets preempted away and this CPU is
> > hotplugged away? You'll reference a CPU ID that does not exist
> > anymore.
>
> You're right of course. But we don't support CPU hotplug on the UV
> hardware. There are enhancements needed in both the BIOS and Linux
> (BAU and GRU among them). They are on our work queue.
But here you put in yet another roadblock.
Thanks,
Ingo
On Wed, Jun 15, 2011 at 06:15:18PM +0200, Ingo Molnar wrote:
>
> * Cliff Wickman <[email protected]> wrote:
>
> > Hi Ingo,
> >
> > On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
> > >
> > > * Cliff Wickman <[email protected]> wrote:
> > >
> > > > On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> > > > > On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <[email protected]> wrote:
> > > > > > From: Cliff Wickman <[email protected]>
> > > > > >
> > > > > > Calling smp_processor_id() from within a preemptable region will issue
> > > > > > a warning if DEBUG_PREEMPT is set.
> > > > > >
> > > > > > Diffed against 3.0.0-rc3
> > > > > >
> > > > > > Signed-off-by: Cliff Wickman <[email protected]>
> > > > > > ---
> > > > > > ?arch/x86/platform/uv/tlb_uv.c | ? ?2 ++
> > > > > > ?1 file changed, 2 insertions(+)
> > > > > >
> > > > > > Index: linux/arch/x86/platform/uv/tlb_uv.c
> > > > > > ===================================================================
> > > > > > --- linux.orig/arch/x86/platform/uv/tlb_uv.c
> > > > > > +++ linux/arch/x86/platform/uv/tlb_uv.c
> > > > > > @@ -1334,7 +1334,9 @@ static ssize_t tunables_write(struct fil
> > > > > >
> > > > > > ? ? ? ?instr[count] = '\0';
> > > > > >
> > > > > > + ? ? ? preempt_disable(); /* avoid DEBUG_PREEMPT warning */
> > > > >
> > > > > I think above code comment, "avoid DEBUG_PREEMPT warning" should be to
> > > > > something more meaningful. It's a BUG, if smp_processor_id() is called
> > > > > within preemptible context. So, we don't want to hit that BUG.
> > > >
> > > > I agree that calling smp_processor_id() within a preemptible context is
> > > > going to produce unpredictable results. In this particular case we just
> > > > need a valid cpu number so that we can find a per-cpu structure.
> > > > That structure contains a reasonable (sanity-checking) limit to the value
> > > > of the tunable that is being written.
> > >
> > > So what happens if the code gets preempted away and this CPU is
> > > hotplugged away? You'll reference a CPU ID that does not exist
> > > anymore.
> >
> > You're right of course. But we don't support CPU hotplug on the UV
> > hardware. There are enhancements needed in both the BIOS and Linux
> > (BAU and GRU among them). They are on our work queue.
>
> But here you put in yet another roadblock.
So would you say I should really widen the scope of the non-preemptible
region to include everything done with the results of that call to
smp_processor_id()?
Which in this case is the call to parse_tunables_write().
Like this:
preempt_disable();
bcp = &per_cpu(bau_control, smp_processor_id());
ret = parse_tunables_write(bcp, instr, count);
preempt_enable_no_resched();
-Cliff
--
Cliff Wickman
SGI
[email protected]
(651) 683-3824
On 06/15/2011 09:40 AM, Cliff Wickman wrote:
>
> So would you say I should really widen the scope of the non-preemptible
> region to include everything done with the results of that call to
> smp_processor_id()?
> Which in this case is the call to parse_tunables_write().
> Like this:
>
> preempt_disable();
> bcp = &per_cpu(bau_control, smp_processor_id());
>
> ret = parse_tunables_write(bcp, instr, count);
> preempt_enable_no_resched();
>
Funny enough, this is such a common pattern that we have helpers for it.
We call this get_cpu() ... put_cpu().
-hpa
On Wed, Jun 15, 2011 at 10:21:42AM -0700, H. Peter Anvin wrote:
> On 06/15/2011 09:40 AM, Cliff Wickman wrote:
> >
> > So would you say I should really widen the scope of the non-preemptible
> > region to include everything done with the results of that call to
> > smp_processor_id()?
> > Which in this case is the call to parse_tunables_write().
> > Like this:
> >
> > preempt_disable();
> > bcp = &per_cpu(bau_control, smp_processor_id());
> >
> > ret = parse_tunables_write(bcp, instr, count);
> > preempt_enable_no_resched();
> >
>
> Funny enough, this is such a common pattern that we have helpers for it.
> We call this get_cpu() ... put_cpu().
>
> -hpa
OK thanks. I'll use them.
I'll fix that patch (1 of 8) and refresh/resend the whole series after a
little time to allow for more reviews of the others.
-Cliff
--
Cliff Wickman
SGI
[email protected]
(651) 683-3824