Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: smp: Optimize delivery of IPIs #69770

Merged
merged 5 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions arch/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ config ARC
imply XIP
select ARCH_HAS_THREAD_LOCAL_STORAGE
select ARCH_SUPPORTS_ROM_START
select ARCH_HAS_DIRECTED_IPIS
help
ARC architecture

Expand All @@ -50,6 +51,7 @@ config ARM64
select USE_SWITCH_SUPPORTED
select IRQ_OFFLOAD_NESTED if IRQ_OFFLOAD
select BARRIER_OPERATIONS_ARCH
select ARCH_HAS_DIRECTED_IPIS
help
ARM64 (AArch64) architecture

Expand Down Expand Up @@ -115,6 +117,7 @@ config RISCV
select USE_SWITCH_SUPPORTED
select USE_SWITCH
select SCHED_IPI_SUPPORTED if SMP
select ARCH_HAS_DIRECTED_IPIS
select BARRIER_OPERATIONS_BUILTIN
imply XIP
help
Expand All @@ -129,6 +132,7 @@ config XTENSA
select ARCH_HAS_CODE_DATA_RELOCATION
select ARCH_HAS_TIMING_FUNCTIONS
select ARCH_MEM_DOMAIN_DATA if USERSPACE
select ARCH_HAS_DIRECTED_IPIS
help
Xtensa architecture

Expand Down Expand Up @@ -746,6 +750,13 @@ config ARCH_HAS_RESERVED_PAGE_FRAMES
memory mappings. The architecture will need to implement
arch_reserved_pages_update().

config ARCH_HAS_DIRECTED_IPIS
bool
help
This hidden configuration should be selected by the architecture if
it has an implementation for arch_sched_directed_ipi() which allows
for IPIs to be directed to specific CPUs.

config CPU_HAS_DCACHE
bool
help
Expand Down
19 changes: 13 additions & 6 deletions arch/arc/core/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <zephyr/kernel.h>
#include <zephyr/kernel_structs.h>
#include <ksched.h>
#include <ipi.h>
#include <zephyr/init.h>
#include <zephyr/irq.h>
#include <arc_irq_offload.h>
Expand Down Expand Up @@ -130,21 +131,27 @@ static void sched_ipi_handler(const void *unused)
z_sched_ipi();
}

/* arch implementation of sched_ipi */
void arch_sched_ipi(void)
void arch_sched_directed_ipi(uint32_t cpu_bitmap)
{
uint32_t i;
unsigned int i;
unsigned int num_cpus = arch_num_cpus();

/* broadcast sched_ipi request to other cores
/* Send sched_ipi request to other cores
* if the target is current core, hardware will ignore it
*/
unsigned int num_cpus = arch_num_cpus();

for (i = 0U; i < num_cpus; i++) {
z_arc_connect_ici_generate(i);
if ((cpu_bitmap & BIT(i)) != 0) {
z_arc_connect_ici_generate(i);
}
evgeniy-paltsev marked this conversation as resolved.
Show resolved Hide resolved
}
}

void arch_sched_broadcast_ipi(void)
Copy link
Member

@TaiJuWu TaiJuWu May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it use ALWAYS_INLINE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arch_sched_broadcast_ipi() I don't think should be using ALWAYS_INLINE as it is not being called from anywhere else in this file. Although arch_sched_directed_ipi() could, I don't think it would get us too much--probably better to let the compiler determine if arch_sched_broadcast_ipi() inlines arch_sched_directed_ipi() or makes a call out to it.

{
arch_sched_directed_ipi(IPI_ALL_CPUS_MASK);
}

int arch_smp_init(void)
{
struct arc_connect_bcr bcr;
Expand Down
2 changes: 0 additions & 2 deletions arch/arc/include/kernel_arch_func.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ extern void z_arc_userspace_enter(k_thread_entry_t user_entry, void *p1,

extern void z_arc_fatal_error(unsigned int reason, const struct arch_esf *esf);

extern void arch_sched_ipi(void);

extern void z_arc_switch(void *switch_to, void **switched_from);

static inline void arch_switch(void *switch_to, void **switched_from)
Expand Down
1 change: 1 addition & 0 deletions arch/arm/core/cortex_a_r/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ config AARCH32_ARMV8_R
bool
select ATOMIC_OPERATIONS_BUILTIN
select SCHED_IPI_SUPPORTED if SMP
select ARCH_HAS_DIRECTED_IPIS
help
This option signifies the use of an ARMv8-R AArch32 processor
implementation.
Expand Down
17 changes: 13 additions & 4 deletions arch/arm/core/cortex_a_r/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <zephyr/kernel.h>
#include <zephyr/arch/arm/cortex_a_r/lib_helpers.h>
#include <zephyr/drivers/interrupt_controller/gic.h>
#include <ipi.h>
#include "boot.h"
#include "zephyr/cache.h"
#include "zephyr/kernel/thread_stack.h"
Expand Down Expand Up @@ -210,7 +211,7 @@ void arch_secondary_cpu_init(void)

#ifdef CONFIG_SMP

static void broadcast_ipi(unsigned int ipi)
static void send_ipi(unsigned int ipi, uint32_t cpu_bitmap)
{
uint32_t mpidr = MPIDR_TO_CORE(GET_MPIDR());

Expand All @@ -220,6 +221,10 @@ static void broadcast_ipi(unsigned int ipi)
unsigned int num_cpus = arch_num_cpus();

for (int i = 0; i < num_cpus; i++) {
if ((cpu_bitmap & BIT(i)) == 0) {
continue;
}

uint32_t target_mpidr = cpu_map[i];
uint8_t aff0;

Expand All @@ -239,10 +244,14 @@ void sched_ipi_handler(const void *unused)
z_sched_ipi();
}

/* arch implementation of sched_ipi */
void arch_sched_ipi(void)
void arch_sched_broadcast_ipi(void)
{
send_ipi(SGI_SCHED_IPI, IPI_ALL_CPUS_MASK);
}

void arch_sched_directed_ipi(uint32_t cpu_bitmap)
{
broadcast_ipi(SGI_SCHED_IPI);
send_ipi(SGI_SCHED_IPI, cpu_bitmap);
}

int arch_smp_init(void)
Expand Down
19 changes: 14 additions & 5 deletions arch/arm64/core/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <zephyr/kernel.h>
#include <zephyr/kernel_structs.h>
#include <ksched.h>
#include <ipi.h>
#include <zephyr/init.h>
#include <zephyr/arch/arm64/mm.h>
#include <zephyr/arch/cpu.h>
Expand Down Expand Up @@ -180,7 +181,7 @@ void arch_secondary_cpu_init(int cpu_num)

#ifdef CONFIG_SMP

static void broadcast_ipi(unsigned int ipi)
static void send_ipi(unsigned int ipi, uint32_t cpu_bitmap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: the platform maintainers might prefer to see each arch get support added in a separate patch instead of all at once, but that's not really for me to say.

{
uint64_t mpidr = MPIDR_TO_CORE(GET_MPIDR());

Expand All @@ -190,6 +191,10 @@ static void broadcast_ipi(unsigned int ipi)
unsigned int num_cpus = arch_num_cpus();

for (int i = 0; i < num_cpus; i++) {
if ((cpu_bitmap & BIT(i)) == 0) {
continue;
}

uint64_t target_mpidr = cpu_map[i];
uint8_t aff0;

Expand All @@ -209,10 +214,14 @@ void sched_ipi_handler(const void *unused)
z_sched_ipi();
}

/* arch implementation of sched_ipi */
void arch_sched_ipi(void)
void arch_sched_broadcast_ipi(void)
{
send_ipi(SGI_SCHED_IPI, IPI_ALL_CPUS_MASK);
}

void arch_sched_directed_ipi(uint32_t cpu_bitmap)
{
broadcast_ipi(SGI_SCHED_IPI);
send_ipi(SGI_SCHED_IPI, cpu_bitmap);
}

#ifdef CONFIG_USERSPACE
Expand All @@ -232,7 +241,7 @@ void mem_cfg_ipi_handler(const void *unused)

void z_arm64_mem_cfg_ipi(void)
{
broadcast_ipi(SGI_MMCFG_IPI);
send_ipi(SGI_MMCFG_IPI, IPI_ALL_CPUS_MASK);
}
#endif

Expand Down
11 changes: 9 additions & 2 deletions arch/riscv/core/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <zephyr/init.h>
#include <zephyr/kernel.h>
#include <ksched.h>
#include <ipi.h>
#include <zephyr/irq.h>
#include <zephyr/sys/atomic.h>
#include <zephyr/arch/riscv/irq.h>
Expand Down Expand Up @@ -86,14 +87,15 @@ static atomic_val_t cpu_pending_ipi[CONFIG_MP_MAX_NUM_CPUS];
#define IPI_SCHED 0
#define IPI_FPU_FLUSH 1

void arch_sched_ipi(void)
void arch_sched_directed_ipi(uint32_t cpu_bitmap)
{
unsigned int key = arch_irq_lock();
unsigned int id = _current_cpu->id;
unsigned int num_cpus = arch_num_cpus();

for (unsigned int i = 0; i < num_cpus; i++) {
if (i != id && _kernel.cpus[i].arch.online) {
if ((i != id) && _kernel.cpus[i].arch.online &&
((cpu_bitmap & BIT(i)) != 0)) {
atomic_set_bit(&cpu_pending_ipi[i], IPI_SCHED);
MSIP(_kernel.cpus[i].arch.hartid) = 1;
}
Expand All @@ -102,6 +104,11 @@ void arch_sched_ipi(void)
arch_irq_unlock(key);
}

void arch_sched_broadcast_ipi(void)
{
arch_sched_directed_ipi(IPI_ALL_CPUS_MASK);
}

#ifdef CONFIG_FPU_SHARING
void arch_flush_fpu_ipi(unsigned int cpu)
{
Expand Down
2 changes: 1 addition & 1 deletion arch/x86/core/intel64/smp.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ int arch_smp_init(void)
* it is not clear exactly how/where/why to abstract this, as it
* assumes the use of a local APIC (but there's no other mechanism).
*/
void arch_sched_ipi(void)
void arch_sched_broadcast_ipi(void)
{
z_loapic_ipi(0, LOAPIC_ICR_IPI_OTHERS, CONFIG_SCHED_IPI_VECTOR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW x86 would seem like an obvious candidate to support this. Local APIC interrupts are trivially targetted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally thinking that that would be outside the scope of this set of changes. I am glad to learn that it is also possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still seem important to get arch layers on board before merge. Again, x86 should be relatively straightforward (and in Intel's wheelhouse, heh). And in any case, getting back to the API naming: having a directed IPI that "actually" does a broadcast seems like the worst of all worlds: we pay the extra scheduler CPU time and latency and get no benefit. If we're going to have architectures that are still broadcast-only, they should never be able to turn on the new code.

}
Expand Down
18 changes: 11 additions & 7 deletions doc/kernel/services/smp/smp.rst
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,17 @@ handle the newly-runnable load.

So where possible, Zephyr SMP architectures should implement an
interprocessor interrupt. The current framework is very simple: the
architecture provides a :c:func:`arch_sched_ipi` call, which when invoked
will flag an interrupt on all CPUs (except the current one, though
that is allowed behavior) which will then invoke the :c:func:`z_sched_ipi`
function implemented in the scheduler. The expectation is that these
APIs will evolve over time to encompass more functionality
(e.g. cross-CPU calls), and that the scheduler-specific calls here
will be implemented in terms of a more general framework.
architecture provides at least a :c:func:`arch_sched_broadcast_ipi` call,
which when invoked will flag an interrupt on all CPUs (except the current one,
though that is allowed behavior). If the architecture supports directed IPIs
(see :kconfig:option:`CONFIG_ARCH_HAS_DIRECTED_IPIS`), then the
architecture also provides a :c:func:`arch_sched_directed_ipi` call, which
when invoked will flag an interrupt on the specified CPUs. When an interrupt is
flagged on the CPUs, the :c:func:`z_sched_ipi` function implmented in the
scheduler will get invoked on those CPUs. The expectation is that these
APIs will evolve over time to encompass more functionality (e.g. cross-CPU
calls), and that the scheduler-specific calls here will be implemented in
terms of a more general framework.

Note that not all SMP architectures will have a usable IPI mechanism
(either missing, or just undocumented/unimplemented). In those cases
Expand Down
12 changes: 10 additions & 2 deletions include/zephyr/arch/arch_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -494,10 +494,18 @@ static inline uint32_t arch_proc_id(void);
/**
* Broadcast an interrupt to all CPUs
*
* This will invoke z_sched_ipi() on other CPUs in the system.
* This will invoke z_sched_ipi() on all other CPUs in the system.
*/
void arch_sched_ipi(void);
void arch_sched_broadcast_ipi(void);

/**
* Direct IPIs to the specified CPUs
*
* This will invoke z_sched_ipi() on the CPUs identified by @a cpu_bitmap.
*
* @param cpu_bitmap A bitmap indicating which CPUs need the IPI
*/
void arch_sched_directed_ipi(uint32_t cpu_bitmap);

int arch_smp_init(void);

Expand Down
4 changes: 2 additions & 2 deletions include/zephyr/kernel_structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ struct z_kernel {
#endif

#if defined(CONFIG_SMP) && defined(CONFIG_SCHED_IPI_SUPPORTED)
/* Need to signal an IPI at the next scheduling point */
bool pending_ipi;
/* Identify CPUs to send IPIs to at the next scheduling point */
atomic_t pending_ipi;
#endif
};

Expand Down
29 changes: 23 additions & 6 deletions kernel/Kconfig.smp
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,11 @@ config MP_MAX_NUM_CPUS
config SCHED_IPI_SUPPORTED
bool
help
True if the architecture supports a call to
arch_sched_ipi() to broadcast an interrupt that will call
z_sched_ipi() on other CPUs in the system. Required for
k_thread_abort() to operate with reasonable latency
(otherwise we might have to wait for the other thread to
take an interrupt, which can be arbitrarily far in the
True if the architecture supports a call to arch_sched_broadcast_ipi()
to broadcast an interrupt that will call z_sched_ipi() on other CPUs
in the system. Required for k_thread_abort() to operate with
reasonable latency (otherwise we might have to wait for the other
thread to take an interrupt, which can be arbitrarily far in the
future).

config TRACE_SCHED_IPI
Expand All @@ -73,6 +72,24 @@ config TRACE_SCHED_IPI
depends on SCHED_IPI_SUPPORTED
depends on MP_MAX_NUM_CPUS>1

config IPI_OPTIMIZE
bool "Optimize IPI delivery"
default n
depends on SCHED_IPI_SUPPORTED && MP_MAX_NUM_CPUS>1
help
When selected, the kernel will attempt to determine the minimum
set of CPUs that need an IPI to trigger a reschedule in response to
a thread newly made ready for execution. This increases the
computation required at every scheduler operation by a value that is
O(N) in the number of CPUs, and in exchange reduces the number of
interrupts delivered. Which to choose is going to depend on
application behavior. If the architecture also supports directing
IPIs to specific CPUs then this has the potential to signficantly
reduce the number of IPIs (and consequently ISRs) processed by the
system as the number of CPUs increases. If not, the only benefit
would be to not issue any IPIs if the newly readied thread is of
lower priority than all the threads currently executing on other CPUs.

config KERNEL_COHERENCE
bool "Place all shared data into coherent memory"
depends on ARCH_HAS_COHERENCE
Expand Down
16 changes: 14 additions & 2 deletions kernel/include/ipi.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,25 @@
#ifndef ZEPHYR_KERNEL_INCLUDE_IPI_H_
#define ZEPHYR_KERNEL_INCLUDE_IPI_H_

#include <zephyr/kernel.h>
#include <stdint.h>
#include <zephyr/sys/atomic.h>

#define IPI_ALL_CPUS_MASK ((1 << CONFIG_MP_MAX_NUM_CPUS) - 1)

#define IPI_CPU_MASK(cpu_id) \
(IS_ENABLED(CONFIG_IPI_OPTIMIZE) ? BIT(cpu_id) : IPI_ALL_CPUS_MASK)


/* defined in ipi.c when CONFIG_SMP=y */
#ifdef CONFIG_SMP
void flag_ipi(void);
void flag_ipi(uint32_t ipi_mask);
void signal_pending_ipi(void);
atomic_val_t ipi_mask_create(struct k_thread *thread);
#else
#define flag_ipi() do { } while (false)
#define flag_ipi(ipi_mask) do { } while (false)
#define signal_pending_ipi() do { } while (false)
#endif /* CONFIG_SMP */


#endif /* ZEPHYR_KERNEL_INCLUDE_IPI_H_ */
Loading
Loading