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

rework IPI handling #685

Closed
wants to merge 1 commit into from
Closed

rework IPI handling #685

wants to merge 1 commit into from

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Dec 2, 2021

Stick to the logic core view in the generic code and resolve the mask at the lowest layer instead. That avoids translating forth an back from a logical core ID to an ID that is just another artificial ID on some architectures. Using the logical core ID as much as possible seem the most simple approach.
This also allows supporting the feature that a target mask can be used down to the lowest level. Just there is's finally decided to implement this.

@axel-h axel-h marked this pull request as draft December 2, 2021 14:17
if (isBlocking) {
while (mask) {
unsigned int index = wordBits - 1 - clzl(mask);
mask &= ~BIT(index);
big_kernel_lock.node_owners[index].ipi = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this supposed to be 0, ie. should the be an assert here?

@axel-h axel-h force-pushed the patch-axel-40 branch 3 times, most recently from 256ade6 to 2379b4c Compare December 5, 2021 23:35
@axel-h axel-h force-pushed the patch-axel-40 branch 2 times, most recently from fc2d559 to 7f14cda Compare December 17, 2021 13:18
@axel-h axel-h force-pushed the patch-axel-40 branch 2 times, most recently from 7eb3da8 to 0b8154b Compare January 10, 2022 03:35
@kent-mcleod
Copy link
Member

What is the motivation for these changes? If this is RISC-V specific, then it may be better to only modify the behavior for RISC-V by changing it's implementation of ipi_send_mask to have its own implementation instead of calling generic_ipi_send_mask.

@axel-h
Copy link
Member Author

axel-h commented Jan 31, 2022

What is the motivation for these changes? If this is RISC-V specific, then it may be better to only modify the behavior for RISC-V by changing it's implementation of ipi_send_mask to have its own implementation instead of calling generic_ipi_send_mask.

This comes from trying to understand what is actually happening on RISC-V and why - and concluding this goes through some forth-and-back translation that are not necessary. The current generic way seems to enforce too much given what architectures are doing in the end. Seem this started at x86, then the parm ARM tried to cope with this and now RISC-V struggles also.
We could drop the generic_ipi_send_mask() and just have a architecture specific ipi_send_mask(). Downside would be that the architectures each have to do the (similar) big_kernel_lock handling then. This is what this PR tries to avoid and keep this part generic.

@kent-mcleod
Copy link
Member

generic_ipi_send_mask() was introduced to allow each architecture to provide it's own optimized IPI send function (40970c3). So if you want to change RISC-V then you can do this without changing the generic function used by other architectures.

That said, changing this code will have subtle effects on system behavior as it would change the timing of how IPIs are sent and received and so any changes really need to be motivated by some sort of demonstration that the existing implementation is insufficient.

@yyshen
Copy link
Contributor

yyshen commented Feb 1, 2022

generic_ipi_send_mask() was introduced to allow each architecture to provide it's own optimized IPI send function (40970c3). So if you want to change RISC-V then you can do this without changing the generic function used by other architectures.

That said, changing this code will have subtle effects on system behavior as it would change the timing of how IPIs are sent and received and so any changes really need to be motivated by some sort of demonstration that the existing implementation is insufficient.

I tend to second this. If we want to optimise the RISCV case, it can be done in a specialised ipi_send_target for RISCV. Could you list the scenarios that multiple IPI targets are required? Also, for GICv3, the function already groups IPI targets by AFFI1.

Stick to the logic core view in the generic code and resolve the mask at
the lowest layer instead.

Signed-off-by: Axel Heider <axelheider@gmx.de>
@Indanz
Copy link
Contributor

Indanz commented Nov 30, 2023

Perhaps good to clarify why I'm closing this: Draft stage is fine for when something is not ready to be merged yet because it still needs some work. But the reason to have it as a pull request is to solicit feedback from others. You got that, but haven't made a strong argument why this should be merged, considering it's not necessary, for almost two years.

To be clear, I fully support reworking the IPI code and think the current code is terrible. That said, I don't think your commit goes far enough. E.g. ipi_send_target is never used with a list, only for a single core. Good cleanups should have significant code reduction.

@axel-h
Copy link
Member Author

axel-h commented Nov 30, 2023

Closing is fine, with #1135 we have a new driver for this now that adds another good argument for having an ipi_send_target() that only takes one singe core, as this seem the only practical use case. Having a function that can address multiple cores efficiently seems just a nice-to-have currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants