[PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Cliff Wickman]

From: Cliff Wickman <cpw@sgi.com>

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 <cpw@sgi.com>
---
 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)

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Rakib Mullick]

On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <cpw@sgi.com> wrote:
> From: Cliff Wickman <cpw@sgi.com>
>
> 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 <cpw@sgi.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Cliff Wickman]

On Wed, Jun 15, 2011 at 12:05:17PM +0600, Rakib Mullick wrote:
> On Wed, Jun 15, 2011 at 5:06 AM, Cliff Wickman <cpw@sgi.com> wrote:
> > From: Cliff Wickman <cpw@sgi.com>
> >
> > 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 <cpw@sgi.com>
> > ---
> > ?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 majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Cliff Wickman
SGI
cpw@sgi.com
(651) 683-3824

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Ingo Molnar]

* Cliff Wickman <cpw@sgi.com> 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 <cpw@sgi.com> wrote:
> > > From: Cliff Wickman <cpw@sgi.com>
> > >
> > > 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 <cpw@sgi.com>
> > > ---
> > > ?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

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Cliff Wickman]

Hi Ingo,

On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
> 
> * Cliff Wickman <cpw@sgi.com> 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 <cpw@sgi.com> wrote:
> > > > From: Cliff Wickman <cpw@sgi.com>
> > > >
> > > > 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 <cpw@sgi.com>
> > > > ---
> > > > ?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
cpw@sgi.com
(651) 683-3824

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Ingo Molnar]

* Cliff Wickman <cpw@sgi.com> wrote:

> Hi Ingo,
> 
> On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
> > 
> > * Cliff Wickman <cpw@sgi.com> 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 <cpw@sgi.com> wrote:
> > > > > From: Cliff Wickman <cpw@sgi.com>
> > > > >
> > > > > 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 <cpw@sgi.com>
> > > > > ---
> > > > > ?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

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Cliff Wickman]

On Wed, Jun 15, 2011 at 06:15:18PM +0200, Ingo Molnar wrote:
> 
> * Cliff Wickman <cpw@sgi.com> wrote:
> 
> > Hi Ingo,
> > 
> > On Wed, Jun 15, 2011 at 05:54:45PM +0200, Ingo Molnar wrote:
> > > 
> > > * Cliff Wickman <cpw@sgi.com> 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 <cpw@sgi.com> wrote:
> > > > > > From: Cliff Wickman <cpw@sgi.com>
> > > > > >
> > > > > > 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 <cpw@sgi.com>
> > > > > > ---
> > > > > > ?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
cpw@sgi.com
(651) 683-3824

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[H. Peter Anvin]

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

Re: [PATCH 1 of 6] x86, UV: smp_processor_id in a preemptable region

[Cliff Wickman]

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
cpw@sgi.com
(651) 683-3824