irq: introduce new `irq_set_affinity()` API - alternate #77828

ycsin · 2024-08-30T16:54:00Z

Note

This implementation doesn't require empty stubs in all of the architectures, at the expense of an additional precompiler switch in the irq.h header

This is an alternate implementation of #77645 that creates empty stubs for each of the architectures, check that PR out for more info.

irq_set_affinity(irq, mask) allows the runtime affinity configuration of an interrupt line, which can be helpful in tuning the performance in a SMP setup (i.e. irqbalance).

An implementation is added for RISC-V PLIC in this PR.

Kconfig dependencies

Testing on `qemu_riscv64_smp`

west build -b qemu_riscv64_smp -p always -t run zephyr/samples/hello_world -- \
  -DCONFIG_SHELL=y \
  -DCONFIG_MP_MAX_NUM_CPUS=4 \
  -DCONFIG_IRQ_AFFINITY=y \
  -DCONFIG_PLIC_SHELL=y

[125/126] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: riscv64
*** Booting Zephyr OS build v3.7.0-1894-g2555dd2bb4f6 ***
Hello World! qemu_riscv64/qemu_virt_riscv64/smp

uart:~$ plic stats get interrupt-controller@c000000 
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         105           0           0           0         123      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         211           0           0           0         229      0x800058fa(0x8000a130)

uart:~$ plic affinity set interrupt-controller@c000000 10 0x2
IRQ 10 affinity set to 0x2
uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         110           0           0         490      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         208           0           0         589      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         315           0           0         695      0x800058fa(0x8000a130)

uart:~$ plic affinity set interrupt-controller@c000000 10 0x4
IRQ 10 affinity set to 0x4
uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         404         100           0         880      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         404         206           0         986      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         404         305           0        1085      0x800058fa(0x8000a130)

uart:~$ plic affinity set interrupt-controller@c000000 10 0x8
IRQ 10 affinity set to 0x8
uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         404         398         114        1287      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
    10         367         404         398         216        1390      0x800058fa(0x8000a130)

uart:~$ plic affinity set interrupt-controller@c000000 10 0xf
IRQ 10 affinity set to 0xF
uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
     0          12          16          14          20          71      0x800045f4(0)
    10         391         432         421         345        1598      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
     0          38          38          40          40         162      0x800045f4(0)
    10         417         457         452         371        1706      0x800058fa(0x8000a130)

uart:~$ plic stats get interrupt-controller@c000000
   IRQ       CPU 0       CPU 1       CPU 2       CPU 3       Total      ISR(ARG)
     0          62          60          65          58         252      0x800045f4(0)
    10         438         484         486         398        1815      0x800058fa(0x8000a130)

uart:~$

andyross

Seems clean. This version with the kconfigs stubbing gets my vote for sure. One nitpick about mismatched bit widths in the riscv code.

drivers/interrupt_controller/intc_plic.c

cfriedt · 2024-09-14T14:41:19Z

drivers/interrupt_controller/intc_plic.c

+		}
+		if (CONFIG_MP_NUM_CPUS > 1) {
+			shell_fprintf(sh, SHELL_NORMAL, "  %5d",
+				      stat.irq_count[arch_num_cpus() * config->nr_irqs + i]);


The indeces into this array are fairly confusing but this line can be simplified slightly using a previously declared variable.

Suggested change

stat.irq_count[arch_num_cpus() * config->nr_irqs + i]);

stat.irq_count[offset + i]);

In the plic_stats structure. Is irq_count a pointer to an array that is 2+ dimensions in size? Or does some other dimension reflect values other than irq counts?

It might be good in a future PR to clean that up a bit.

Reviewed #78140. I think it's a bit clearer now. Is this an interleaved array based on hart contexts?

For every instance of a PLIC, if PLIC_SHELL is enabled, there will be a local_irq_count[NUM_CPUS][NUM_IRQS], which is pointed to by stat.irq_count.

I can probably make a (per instance) helper inline function to access this so that it is clearer

con-pax · 2024-09-16T13:40:03Z

Hey @ycsin
I wanted to test this before I go on PTO, am I missing something?
build command:

$ git log --oneline HEAD~1..HEAD
b257c44d6f5 (HEAD -> ycsin-affinity-smp, ycsin/pr/irq_set_affinity-1) drivers: intc: plic: implement `irq_set_affinity()`
$ cd ../
$ west build -p -b mpfs_icicle/polarfire/u54/smp zephyr/samples/hello_world/ -DCONFIG_SHELL=y -DCONFIG_PLIC_SHELL=y

output:

*** Booting Zephyr OS build v3.7.0-2739-gb257c44d6f56 ***
Hello World! mpfs_icicle/polarfire/u54/smp

uart:~$ plic stats get interrupt-controller@c000000
   IRQ  CPU 0  CPU 1  CPU 2  CPU 3  Total       ISR(ARG)
    91      0     38      0      0     41       0x800058ec(0x8000a178)

uart:~$ plic affinity set interrupt-controller@c000000 91 0x2
plic - PLIC shell commands
Subcommands:
  stats  : IRQ stats
uart:~$

ycsin · 2024-09-16T13:43:06Z

am I missing something?

Sorry, I forgot to update the build command - you need to add CONFIG_IRQ_AFFINITY=y

con-pax · 2024-09-16T13:46:47Z

Sorry, I forgot to update the build command - you need to add CONFIG_IRQ_AFFINITY=y

Thanks! I though that and tried it however I got a bit of vomit:

$ west build -p -b mpfs_icicle/polarfire/u54/smp zephyr/samples/hello_world/ -DCONFIG_SHELL=y -DCONFIG_PLIC_SHELL=y -DCONFIG_PLIC_IRQ_AFFINITY=y
-- west build: making build dir /mnt/dev/z_workspace/build pristine
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: /mnt/dev/z_workspace/zephyr/samples/hello_world
-- CMake version: 3.28.0-rc5
-- Found Python3: /usr/bin/python3 (found suitable version "3.8.10", minimum required is "3.8") found components: Interpreter 
-- Cache files will be written to: /home/conor/.cache/zephyr
-- Zephyr version: 3.7.99 (/mnt/dev/z_workspace/zephyr)
-- Found west (found suitable version "1.2.0", minimum required is "0.14.0")
-- Board: mpfs_icicle, qualifiers: polarfire/u54/smp
-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
-- Found host-tools: zephyr 0.16.8 (/home/conor/zephyr-sdk-0.16.8)
-- Found toolchain: zephyr 0.16.8 (/home/conor/zephyr-sdk-0.16.8)
-- Found Dtc: /home/conor/zephyr-sdk-0.16.8/sysroots/x86_64-pokysdk-linux/usr/bin/dtc (found suitable version "1.6.0", minimum required is "1.4.6") 
-- Found BOARD.dts: /mnt/dev/z_workspace/zephyr/boards/microchip/mpfs_icicle/mpfs_icicle_polarfire_u54_smp.dts
-- Generated zephyr.dts: /mnt/dev/z_workspace/build/zephyr/zephyr.dts
-- Generated devicetree_generated.h: /mnt/dev/z_workspace/build/zephyr/include/generated/zephyr/devicetree_generated.h
-- Including generated dts.cmake file: /mnt/dev/z_workspace/build/zephyr/dts.cmake
Parsing /mnt/dev/z_workspace/zephyr/Kconfig
Loaded configuration '/mnt/dev/z_workspace/zephyr/boards/microchip/mpfs_icicle/mpfs_icicle_polarfire_u54_smp_defconfig'
Merged configuration '/mnt/dev/z_workspace/zephyr/samples/hello_world/prj.conf'
Merged configuration '/mnt/dev/z_workspace/build/zephyr/misc/generated/extra_kconfig_options.conf'

error: PLIC_IRQ_AFFINITY (defined at drivers/interrupt_controller/Kconfig.plic:17) is assigned in a
configuration file, but is not directly user-configurable (has no prompt). It gets its value
indirectly from other symbols. See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_PLIC_IRQ_AFFINITY and/or look up
PLIC_IRQ_AFFINITY in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.

CMake Error at /mnt/dev/z_workspace/zephyr/cmake/modules/kconfig.cmake:389 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /mnt/dev/z_workspace/zephyr/cmake/modules/zephyr_default.cmake:133 (include)
  /mnt/dev/z_workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /mnt/dev/z_workspace/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:92 (include_boilerplate)
  CMakeLists.txt:5 (find_package)

con-pax · 2024-09-16T14:18:34Z

Whoops! my bad, CONFIG_IRQ_AFFINITY!

*** Booting Zephyr OS build v3.7.0-2739-gb257c44d6f56 ***
Hello World! mpfs_icicle/polarfire/u54/smp

uart:~$ plic stats get interrupt-controller@c000000
   IRQ  CPU 0  CPU 1  CPU 2  CPU 3  Total       ISR(ARG)
    91      0     38      0      0     41       0x80005c6a(0x8000a4f8)

uart:~$ plic affinity set interrupt-controller@c000000 91 0x2
IRQ 91 affinity set to 0x2
uart:~$ plic stats get interrupt-controller@c000000
   IRQ  CPU 0  CPU 1  CPU 2  CPU 3  Total       ISR(ARG)
    91      0     87    433      0    522       0x80005c6a(0x8000a4f8)

uart:~$ plic affinity set interrupt-controller@c000000 91 0x4
IRQ 91 affinity set to 0x4
uart:~$ plic stats get interrupt-controller@c000000
   IRQ  CPU 0  CPU 1  CPU 2  CPU 3  Total       ISR(ARG)
    91      0     87    471     39    598       0x80005c6a(0x8000a4f8)

uart:~$

This looks really cool to me!

con-pax · 2024-09-16T14:19:48Z

I'm not around for a couple of weeks, but this deffo gets my vote

ycsin · 2024-09-16T15:16:00Z

I'm not around for a couple of weeks, but this deffo gets my vote

Have a good one! 🏖️
And thanks for testing.

`irq_set_affinity(irq, mask)` allows the runtime affinity configuration of an interrupt line, which can be helpful in tuning the performance in a SMP setup (i.e. `irqbalance`). Signed-off-by: Yong Cong Sin <[email protected]> Signed-off-by: Yong Cong Sin <[email protected]>

- Implement `irq_set_affinity()` in RISCV PLIC. - Added new affinity shell command to get/set the irq(s) affinity in runtime, when `0` is sent as the `local_irq`, it means set/get all IRQs affinity. - Some minor optimizations Updated the build_all test to build this new configuration. Signed-off-by: Yong Cong Sin <[email protected]> Signed-off-by: Yong Cong Sin <[email protected]>

ycsin · 2024-09-17T03:03:02Z

Rebased after #78140

bjarki-andreasen

Looks good :)

ycsin · 2024-09-19T05:16:30Z

Having looked at a few more interrupt controller drivers and the (custom) APIs they implement, I'm not sure if this PR is the best path forward - the configuration of IRQ affinity should not be something special when there are custom APIs to set/clear pending and so on. Creating a new arch-level API might not be necessary since PLIC will be the only controller using it for the foreseeable future, and does little to the ad-hoc nature of our interrupt controller implementation.

I'm going to spin off the PLIC IRQ affinity implementation into a separate PR that can be reviewed separately from the introduction of irq_set_affinity() - #78681

Comments welcomed.

ycsin added area: Kernel area: RISCV RISCV Architecture (32-bit & 64-bit) RFC Request For Comments: want input from the community area: Architectures labels Aug 30, 2024

ycsin requested a review from cfriedt August 30, 2024 16:54

This was referenced Aug 30, 2024

[DNM][RFC] irq: introduce new irq_set_affinity() API #77645

Closed

drivers: intc: plic: fix IRQ on all CPUs #77116

Closed

ycsin force-pushed the pr/irq_set_affinity-1 branch 2 times, most recently from 9e51572 to 94e7a47 Compare August 31, 2024 08:46

ycsin marked this pull request as ready for review August 31, 2024 13:09

ycsin requested review from dcpleung and nashif as code owners August 31, 2024 13:09

zephyrbot added area: Interrupt Controller area: Devicetree labels Aug 31, 2024

zephyrbot requested review from carlocaione, decsny, edersondisouza, fkokosinski, galak, katsuster, kgugala, mgielda, npitre and tgorochowik August 31, 2024 13:10

zephyrbot assigned fkokosinski, kgugala and tgorochowik Aug 31, 2024

andyross previously approved these changes Sep 5, 2024

View reviewed changes

drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved

ycsin dismissed andyross’s stale review via 711eddc September 5, 2024 09:43

ycsin force-pushed the pr/irq_set_affinity-1 branch from 94e7a47 to 711eddc Compare September 5, 2024 09:43

ycsin force-pushed the pr/irq_set_affinity-1 branch from b02f092 to d86dff1 Compare September 10, 2024 04:11

ycsin mentioned this pull request Sep 10, 2024

irq: improve API consistency #78199

Open

ycsin changed the title ~~[DNM][RFC] irq: introduce new irq_set_affinity() API - alternate~~ irq: introduce new irq_set_affinity() API - alternate Sep 10, 2024

ycsin mentioned this pull request Sep 12, 2024

drivers: intc: plic: fix IRQ on every hart regardless of mapping #78140

Merged

ycsin added the DNM This PR should not be merged (Do Not Merge) label Sep 12, 2024

ycsin force-pushed the pr/irq_set_affinity-1 branch 2 times, most recently from b4142f4 to 2e36ff4 Compare September 13, 2024 09:11

cfriedt reviewed Sep 14, 2024

View reviewed changes

ycsin force-pushed the pr/irq_set_affinity-1 branch from 2e36ff4 to b257c44 Compare September 15, 2024 16:20

ycsin added 2 commits September 17, 2024 11:01

ycsin force-pushed the pr/irq_set_affinity-1 branch from b257c44 to 1d60d74 Compare September 17, 2024 03:01

ycsin removed RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Sep 17, 2024

ycsin mentioned this pull request Sep 17, 2024

arch: riscv: smp: refactor IPI functions and guard with CONFIG_RISCV_SMP_IPI_CLINT #78493

Closed

1 task

ycsin requested review from cfriedt, evgeniy-paltsev and andyross and removed request for decsny September 17, 2024 14:33

bjarki-andreasen approved these changes Sep 17, 2024

View reviewed changes

ycsin mentioned this pull request Sep 19, 2024

drivers: intc: plic: implement irq affinity configuration #78681

Merged

ycsin closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

irq: introduce new `irq_set_affinity()` API - alternate #77828

irq: introduce new `irq_set_affinity()` API - alternate #77828

Uh oh!

ycsin commented Aug 30, 2024 •

edited

Loading

Uh oh!

andyross left a comment

Uh oh!

Uh oh!

cfriedt Sep 14, 2024

Uh oh!

cfriedt Sep 15, 2024

Uh oh!

ycsin Sep 15, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

ycsin commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

ycsin commented Sep 16, 2024 •

edited

Loading

Uh oh!

ycsin commented Sep 17, 2024 •

edited

Loading

Uh oh!

bjarki-andreasen left a comment

Uh oh!

ycsin commented Sep 19, 2024

Uh oh!

Uh oh!

	stat.irq_count[arch_num_cpus() * config->nr_irqs + i]);
	stat.irq_count[offset + i]);

irq: introduce new irq_set_affinity() API - alternate #77828

irq: introduce new irq_set_affinity() API - alternate #77828

Uh oh!

Conversation

ycsin commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Kconfig dependencies

Testing on qemu_riscv64_smp

Uh oh!

andyross left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfriedt Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

cfriedt Sep 15, 2024

Choose a reason for hiding this comment

Uh oh!

ycsin Sep 15, 2024

Choose a reason for hiding this comment

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

ycsin commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

con-pax commented Sep 16, 2024

Uh oh!

ycsin commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycsin commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Uh oh!

ycsin commented Sep 19, 2024

Uh oh!

Uh oh!

irq: introduce new `irq_set_affinity()` API - alternate #77828

irq: introduce new `irq_set_affinity()` API - alternate #77828

ycsin commented Aug 30, 2024 •

edited

Loading

Testing on `qemu_riscv64_smp`

ycsin commented Sep 16, 2024 •

edited

Loading

ycsin commented Sep 17, 2024 •

edited

Loading