Skip to content

arch: support customized up_cpu_index() in AMP mode#13886

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_15
Oct 9, 2024
Merged

arch: support customized up_cpu_index() in AMP mode#13886
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_15

Conversation

@hujun260
Copy link
Copy Markdown
Contributor

@hujun260 hujun260 commented Oct 8, 2024

Summary

support customized up_cpu_index() in AMP mode

Some app with same code runs on different cores in AMP mode, need the physical core on which the function is called.

Impact

arm

Testing

ostest

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Oct 8, 2024
@nuttxpr
Copy link
Copy Markdown

nuttxpr commented Oct 8, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and how to improve it:

Missing Information:

  • Summary:
    • Insufficient Detail: The "How" is vague. Explain exactly how up_cpu_index() is customized and accessed within the AMP context. Provide a code example if possible.
    • No Issue References: Link any related NuttX issues this PR addresses.
  • Impact:
    • Incomplete:
      • You mention "arm" but don't specify architectures or boards. Be specific about affected hardware.
      • Address all impact points (build process, documentation, security, compatibility, etc.). Even if the answer is "NO", state it explicitly.
  • Testing:
    • Inadequate:
      • Build Host: Specify your development environment's OS, CPU architecture, and compiler version.
      • Target(s): Be very specific about the ARM architectures and boards you tested on. Provide the board configuration names (e.g., board:config).
      • Logs: "ostest" is not descriptive. Include relevant snippets of logs from before and after your changes to demonstrate the problem and the fix.

How to Improve:

  1. Expand the Summary: Provide a clear, step-by-step explanation of your changes to up_cpu_index(). Use code examples to illustrate the new behavior.
  2. Complete the Impact Assessment: Go through each impact point and provide a "YES" or "NO" answer with a brief explanation.
  3. Provide Detailed Testing Information:
    • Specify your build host environment.
    • List all tested ARM architectures, boards, and their configurations.
    • Include relevant log snippets that show the issue before the change and the correct behavior after.

Remember: A well-documented PR significantly increases its chances of being reviewed and merged quickly. Be thorough and provide as much context as possible!

Comment thread arch/arm/src/common/arm_cpuindex.c Outdated
@github-actions github-actions Bot added Size: S The size of the change in this PR is small and removed Size: M The size of the change in this PR is medium labels Oct 8, 2024
Comment thread arch/arm/include/arm/irq.h Outdated
Comment thread arch/arm/include/tlsr82/irq.h
@github-actions github-actions Bot added Arch: ceva Issues related to CEVA architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: sparc Issues related to the SPARC architecture Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Area: OS Components OS Components issues labels Oct 8, 2024
Comment thread sched/Kconfig Outdated
Comment thread sched/Kconfig Outdated
Comment thread arch/arm/include/arm/irq.h Outdated
Comment thread sched/Kconfig Outdated
@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 8, 2024

Isn't up_cpu_index the logical core number (which is only meaningful to the NuttX kernel), not the physical core number? I presume for AMP communication you need the physical core.

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

xiaoxiang781216 commented Oct 8, 2024

Isn't up_cpu_index the logical core number (which is only meaningful to the NuttX kernel), not the physical core number? I presume for AMP communication you need the physical core.

Yes, that's why we have two api now: this_cpu and up_cpu_index. this_cpu is logical core number, up_cpu_index is physical core number.
up_cpu_index always return the real core number regardless CONFIG_SMP setting, on the other hand this_cpu always return 0 if CONFIG_SMP is false after this patch: #13211

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 8, 2024

Isn't up_cpu_index the logical core number (which is only meaningful to the NuttX kernel), not the physical core number? I presume for AMP communication you need the physical core.

Yes, that's why we have two api now: this_cpu and up_cpu_index. this_cpu is logical core number, up_cpu_index is physical core number. up_cpu_index always return the real core number regardless CONFIG_SMP setting, on the other hand this_cpu always return 0 if CONFIG_SMP is false after this patch: #13211

This makes sense, but the function description of up_cpu_index is still talking about SMP / logical CPU number, you need to update that too.

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

@hujun260 look at @pussuw 's suggestion.

@github-actions github-actions Bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture labels Oct 9, 2024
@hujun260 hujun260 force-pushed the apache_15 branch 3 times, most recently from d2b6d09 to 32f1a0d Compare October 9, 2024 05:54
@github-actions github-actions Bot removed the Area: OS Components OS Components issues label Oct 9, 2024
Comment thread arch/arm/src/sam34/sam4cm_cpuindex.c
Comment thread arch/xtensa/src/esp32s3/esp32s3_cpuindex.S
Comment thread arch/xtensa/src/esp32/esp32_cpuindex.S
Comment thread arch/arm/src/cxd56xx/cxd56_cpuindex.c
Comment thread arch/arm/src/lc823450/lc823450_cpuindex.c
Comment thread arch/arm64/src/zynq-mpsoc/zynq_boot.c
Comment thread arch/risc-v/src/common/riscv_cpuindex.c
Comment thread arch/sim/src/sim/posix/sim_hostsmp.c Outdated
Comment thread arch/sparc/src/s698pm/s698pm_cpuindex.c
Comment thread include/nuttx/spinlock.h
@hujun260 hujun260 force-pushed the apache_15 branch 2 times, most recently from 9637c97 to c96e54f Compare October 9, 2024 07:52
@github-actions github-actions Bot added Area: Networking Effects networking subsystem Area: OS Components OS Components issues labels Oct 9, 2024
@hujun260 hujun260 force-pushed the apache_15 branch 2 times, most recently from 7ffb4ab to 79cf4ab Compare October 9, 2024 10:54
Comment thread arch/sim/src/sim/CMakeLists.txt Outdated
Some app with same code runs on different cores in AMP mode,
need the physical core on which the function is called.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
Copy link
Copy Markdown
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

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

LGTM

@xiaoxiang781216 xiaoxiang781216 merged commit e249dd2 into apache:master Oct 9, 2024
list(APPEND SRCS sim_smpsignal.c sim_cpuidlestack.c)
endif()

if(CONFIG_ARCH_HAVE_MULTICPU)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hujun260 only POSIX implementation is provided here, no Win implementation
this will block MSVC build

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's check windows before select ARCH_HAVE_MULTICPU, @hujun260

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

@hujun260 hujun260 deleted the apache_15 branch October 10, 2024 05:32
hujun260 added a commit to hujun260/nuttx that referenced this pull request Oct 10, 2024
This commit fixes the regression from apache#13886

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@hujun260
Copy link
Copy Markdown
Contributor Author

#14024

xiaoxiang781216 pushed a commit that referenced this pull request Oct 10, 2024
This commit fixes the regression from #13886

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

@hujun260 @xiaoxiang781216 do you have a plan to introduce conversion between logical <-> physical CPU index via the this_cpu() macro in the future ? Now this_cpu() is still equal to up_cpu_index().

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

@hujun260 @xiaoxiang781216 do you have a plan to introduce conversion between logical <-> physical CPU index via the this_cpu() macro in the future ?

what's conversion do you want?

Now this_cpu() is still equal to up_cpu_index().

it isn't always equals, when SMP is off, up_cpu_index may return nonzero value, but this_cpu always return zero.

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

A logical CPU index just an arbitrary number and its value is only meaningful for the operating system code, while the physical CPU index can have meaning in things like interrupt acknowledgement etc. They most definitely do not have to be the same, even though in our case they are, at least atm.

The issue arises when we introduce a platform, that has CPU cores with unequal capabilities e.g. MPFS / Polarfire SoC. It has harts 0..4 but hart0 cannot participate in NuttX SMP as it is less capable than harts 1..4, it does not have an MMU for one example. On the other hand, for interrupt acknowledgment the physical core (hartID) is needed.

This is why some kind of logical <-> physical conversion (other than physical=logical) is inevitably needed. I think Linux handles this via FDT, but we can do with something much simpler.

My question was simply to ask if you were already implementing something for this_cpu(). If not, then I'll do the simple translation that fixes MPFS SMP.

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

No, this type of mapping can only be handled by arch/chip specific code.

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

Yep, I'll add a hook under arch for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: ceva Issues related to CEVA architecture Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: renesas Issues related to the Renesas chips Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: sparc Issues related to the SPARC architecture Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Arch: z16 Issues related to the Z16 architecture Area: Networking Effects networking subsystem Area: OS Components OS Components issues Size: L The size of the change in this PR is large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants