All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tony Lindgren <tony@atomide.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: v.narang@samsung.com, a.sahrawat@samsung.com,
	Andrew Morton <akpm@linux-foundation.org>,
	Marc Zyngier <maz@kernel.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Valentin Schneider <valentin.schneider@arm.com>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Maninder Singh <maninder1.s@samsung.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Nathan Huckleberry <nhuck@google.com>,
	Will Deacon <will@kernel.org>, Jian Cai <caij2003@gmail.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/3] arm: introduce IRQ stacks
Date: Mon, 9 Nov 2020 16:45:49 +0200	[thread overview]
Message-ID: <20201109144549.GA26857@atomide.com> (raw)
In-Reply-To: <CAK8P3a3s9JJpeBpH38utw9aA1VaEkcBqKEGtwcmoP1zS6xDj5Q@mail.gmail.com>

* Arnd Bergmann <arnd@kernel.org> [201021 16:07]:
> On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> > >
> > > That means we need to also code that up in assembly - remember, we
> > > need to access thread_info from assembly code.
> >
> > Note also that there is a circular dependency involved. If you make
> > thread_info accessible via per-cpu, then:
> >
> > #ifndef __my_cpu_offset
> > #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> > #endif
> > #ifdef CONFIG_DEBUG_PREEMPT
> > #define my_cpu_offset per_cpu_offset(smp_processor_id())
> > #else
> > #define my_cpu_offset __my_cpu_offset
> > #endif
> 
> Right, I had missed the fallback path using asm-generic/percpu.h
> that is used with CONFIG_SMP && CONFIG_CPU_V6
> Almost everything either uses fixed percpu data (on UP builds)
> or TPIDRPRW when building a v7-only or v6k/v7 kernel without
> v6 support.
> 
> > smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> > is:
> >
> > #define raw_smp_processor_id() (current_thread_info()->cpu)
> >
> > and if current_thread_info() itself involves reading from per-cpu data,
> > we end up recursing... infinitely.
> >
> > This is why I said in the other thread:
> >
> > "We don't do it because we don't have a separate register to be able
> > to store the thread_info pointer, and copying that lump between the
> > SVC and IRQ stack will add massively to IRQ latency, especially for
> > older machines."
> 
> As discussed on IRC, I think it can still be done in one of these
> ways, though admittedly none of them are perfect:
> 
> a) add runtime patching for __my_cpu_offset() when
>   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
>   fallback for for SMP&&CPU_V6. It possibly also speeds up
>   running on single-cpu systems if the TPIDRPRW access adds
>   any measurable runtime overhead compared to patching it out.

Out of these options a) sounds best to me.

> b) If irq stacks are left as a compile-time option, that could be
>   made conditional on "!(SMP&&CPU_V6)". Presumably very
>   few people still run kernels built that way any more. The only
>   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
>   which are fairly uncommon these days and would usually
>   run v6-only non-SMP kernels.

This has been working just fine for years though. In general,
removing the conditional compile ifdefferey has made things quite
a bit easier for us, so let's continue on that.

> c) If we decide that we no longer care about that configuration
>   at all, we could decide to just make SMP depend on !CPU_V6,
>   and possibly kill off the entire SMP_ON_UP patching logic.
>   I suspect we still want to keep SMP_ON_UP for performance
>   reasons, but I don't know how significant they are to start with.

And this too has been working just fine for years :)

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Tony Lindgren <tony@atomide.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	v.narang@samsung.com, a.sahrawat@samsung.com,
	Marc Zyngier <maz@kernel.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Nathan Huckleberry <nhuck@google.com>,
	Jian Cai <caij2003@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Maninder Singh <maninder1.s@samsung.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	Valentin Schneider <valentin.schneider@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/3] arm: introduce IRQ stacks
Date: Mon, 9 Nov 2020 16:45:49 +0200	[thread overview]
Message-ID: <20201109144549.GA26857@atomide.com> (raw)
In-Reply-To: <CAK8P3a3s9JJpeBpH38utw9aA1VaEkcBqKEGtwcmoP1zS6xDj5Q@mail.gmail.com>

* Arnd Bergmann <arnd@kernel.org> [201021 16:07]:
> On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> > >
> > > That means we need to also code that up in assembly - remember, we
> > > need to access thread_info from assembly code.
> >
> > Note also that there is a circular dependency involved. If you make
> > thread_info accessible via per-cpu, then:
> >
> > #ifndef __my_cpu_offset
> > #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> > #endif
> > #ifdef CONFIG_DEBUG_PREEMPT
> > #define my_cpu_offset per_cpu_offset(smp_processor_id())
> > #else
> > #define my_cpu_offset __my_cpu_offset
> > #endif
> 
> Right, I had missed the fallback path using asm-generic/percpu.h
> that is used with CONFIG_SMP && CONFIG_CPU_V6
> Almost everything either uses fixed percpu data (on UP builds)
> or TPIDRPRW when building a v7-only or v6k/v7 kernel without
> v6 support.
> 
> > smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> > is:
> >
> > #define raw_smp_processor_id() (current_thread_info()->cpu)
> >
> > and if current_thread_info() itself involves reading from per-cpu data,
> > we end up recursing... infinitely.
> >
> > This is why I said in the other thread:
> >
> > "We don't do it because we don't have a separate register to be able
> > to store the thread_info pointer, and copying that lump between the
> > SVC and IRQ stack will add massively to IRQ latency, especially for
> > older machines."
> 
> As discussed on IRC, I think it can still be done in one of these
> ways, though admittedly none of them are perfect:
> 
> a) add runtime patching for __my_cpu_offset() when
>   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
>   fallback for for SMP&&CPU_V6. It possibly also speeds up
>   running on single-cpu systems if the TPIDRPRW access adds
>   any measurable runtime overhead compared to patching it out.

Out of these options a) sounds best to me.

> b) If irq stacks are left as a compile-time option, that could be
>   made conditional on "!(SMP&&CPU_V6)". Presumably very
>   few people still run kernels built that way any more. The only
>   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
>   which are fairly uncommon these days and would usually
>   run v6-only non-SMP kernels.

This has been working just fine for years though. In general,
removing the conditional compile ifdefferey has made things quite
a bit easier for us, so let's continue on that.

> c) If we decide that we no longer care about that configuration
>   at all, we could decide to just make SMP depend on !CPU_V6,
>   and possibly kill off the entire SMP_ON_UP patching logic.
>   I suspect we still want to keep SMP_ON_UP for performance
>   reasons, but I don't know how significant they are to start with.

And this too has been working just fine for years :)

Regards,

Tony

  reply	other threads:[~2020-11-09 14:46 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20201008071628epcas5p24d196a6023a47a3b0bfa7b7f231ec811@epcas5p2.samsung.com>
2020-10-08  7:15 ` [PATCH 0/3] IRQ stack support for ARM Maninder Singh
2020-10-08  7:15   ` Maninder Singh
2020-10-08  7:15   ` [PATCH 1/3] arm: introduce self pointer in thread info Maninder Singh
2020-10-08  7:15     ` Maninder Singh
2020-10-08  7:15   ` [PATCH 2/3] arm: introduce IRQ stacks Maninder Singh
2020-10-08  7:15     ` Maninder Singh
     [not found]     ` <CAK8P3a2RYeNiTy9QmwFVKtFifXxWc9XfAT6ThPoSH9wGYsKGpA@mail.gmail.com>
2020-10-21 12:42       ` Arnd Bergmann
2020-10-21 12:42         ` Arnd Bergmann
2020-10-21 12:45         ` Russell King - ARM Linux admin
2020-10-21 12:45           ` Russell King - ARM Linux admin
2020-10-21 12:55           ` Arnd Bergmann
2020-10-21 12:55             ` Arnd Bergmann
2020-10-21 12:57           ` Russell King - ARM Linux admin
2020-10-21 12:57             ` Russell King - ARM Linux admin
2020-10-21 13:46             ` Vaneet Narang
2020-10-21 13:46               ` Vaneet Narang
2020-10-21 15:59             ` Arnd Bergmann
2020-10-21 15:59               ` Arnd Bergmann
2020-11-09 14:45               ` Tony Lindgren [this message]
2020-11-09 14:45                 ` Tony Lindgren
2020-11-09 19:10                 ` Arnd Bergmann
2020-11-09 19:10                   ` Arnd Bergmann
2020-11-10  9:19                   ` Tony Lindgren
2020-11-10  9:19                     ` Tony Lindgren
2020-11-10 10:04                     ` Arnd Bergmann
2020-11-10 10:04                       ` Arnd Bergmann
2020-11-10 12:04                       ` Tony Lindgren
2020-11-10 12:04                         ` Tony Lindgren
2020-11-10 13:35                         ` Arnd Bergmann
2020-11-10 13:35                           ` Arnd Bergmann
2020-11-11  6:56                           ` Tony Lindgren
2020-11-11  6:56                             ` Tony Lindgren
2020-10-08  7:15   ` [PATCH 3/3] arm: Modify stack trace and dump for use with irq stack Maninder Singh
2020-10-08  7:15     ` Maninder Singh
2020-10-08  7:53   ` [PATCH 0/3] IRQ stack support for ARM Sebastian Andrzej Siewior
2020-10-08  7:53     ` Sebastian Andrzej Siewior
2020-10-08  8:30   ` Russell King - ARM Linux admin
2020-10-08  8:30     ` Russell King - ARM Linux admin
2020-10-15 20:59     ` Nick Desaulniers
2020-10-15 20:59       ` Nick Desaulniers
2020-10-15 21:16       ` Florian Fainelli
2020-10-15 21:16         ` Florian Fainelli
     [not found]     ` <CAK8P3a0h=D8_Kn_fpHbsik_jf4to2jayxj7K7B7=HaNFzKqNnw@mail.gmail.com>
2020-10-21 11:58       ` Arnd Bergmann
2020-10-21 11:58         ` Arnd Bergmann
2020-10-21 12:34         ` Russell King - ARM Linux admin
2020-10-21 12:34           ` Russell King - ARM Linux admin
2020-10-21 12:46           ` Arnd Bergmann
2020-10-21 12:46             ` Arnd Bergmann
2020-10-08 13:04 [PATCH 2/3] arm: introduce IRQ stacks kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201109144549.GA26857@atomide.com \
    --to=tony@atomide.com \
    --cc=0x7f454c46@gmail.com \
    --cc=a.sahrawat@samsung.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@kernel.org \
    --cc=bigeasy@linutronix.de \
    --cc=caij2003@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=maninder1.s@samsung.com \
    --cc=maz@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=nhuck@google.com \
    --cc=tglx@linutronix.de \
    --cc=v.narang@samsung.com \
    --cc=valentin.schneider@arm.com \
    --cc=vincent.whitchurch@axis.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.