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

test: never disable SMP #34998

Conversation

IRISZZW
Copy link
Contributor

@IRISZZW IRISZZW commented May 8, 2021

To turn SMP on pervasively. Tests should be treated with a combination
of flagging specific cases as "1cpu" where we have short-running tests
that can be independently run in an otherwise SMP environment, and via
setting CONFIG_MP_NUM_CPUS=1 where that's not possible (which still
runs the full SMP kernel config, but with only one CPU available).

We have similar fix, commit: c0e3b92, PR: #18531

Copy link
Collaborator

@abrodkin abrodkin left a comment

Choose a reason for hiding this comment

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

To me provided description looks a bit unclear. I think what was really meant is that SMP is not a property of software configuration, but rather how real hardware is configured. That said it's SoC which should define if it's UP or SMP and then nobody else should override that.

Now given we have some tests which we'd like to execute in subsequent manner and maybe even on the same one CPU core (regardless of how many of those we have to our disposal), that might be solved with limit of CPU cores used in the SMP cluster. And that's what is being implemented here.

Still I'm not sure if all supported in Zephyr SMP SoCs may safely run with reduced amount of cores. ARC platforms are perfectly capable of that, but what about all the rest?

That said IMHO at least wording of the rationale should be improved.

@zephyrbot zephyrbot added area: POSIX POSIX API Library area: Sockets Networking sockets area: Tracing Tracing area: Portability Standard compliant code, toolchain abstraction labels May 11, 2021
@pfalcon
Copy link
Contributor

pfalcon commented May 12, 2021

That said IMHO at least wording of the rationale should be improved.

+1. And there's a typo straight in the subject line: "nerver".

Regarding changes to tests/net/socket/tls* tests done here: I'm not sure why they have CONFIG_SMP=n in the first place (@rlubos, do you have an idea?), but those tests are intended to be unit tests for just socket TLS functionality, so just as well can stays as they are. (Unless there's a specific issue with CONFIG_SMP=n, which I agree is not communicated well in this patch, and would be a deeper issue than just removing it from tests. Otherwise, for as longs as we have CONFIG_SMP config option, it only makes sense to have tests with it being n).

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

Commented above.

@abrodkin
Copy link
Collaborator

Otherwise, for as long as we have CONFIG_SMP config option, it only makes sense to have tests with it being n.
Not clear what do you mean by that. Could you please elaborate a bit more on that?

@IRISZZW IRISZZW added this to the v2.6.0 milestone May 18, 2021
@IRISZZW
Copy link
Contributor Author

IRISZZW commented May 21, 2021

@andyross Any comments about this PR?

@abrodkin abrodkin changed the title test: nerver disable SMP test: never disable SMP May 25, 2021
@galak galak removed this from the v2.6.0 milestone Jun 4, 2021
@cfriedt
Copy link
Member

cfriedt commented Jun 8, 2021

I would +1 a request for a clearer description, but I can see why this could be useful.

At risk of doubling the number of tests twister has to go through, maybe it's reasonable to get smp vs non-smp variants in yaml?

@abrodkin abrodkin added this to the v2.7.0 milestone Jun 8, 2021
@evgeniy-paltsev evgeniy-paltsev added area: ARC ARC Architecture area: SMP Symmetric multiprocessing labels Jun 8, 2021
@abrodkin
Copy link
Collaborator

@IRISZZW could you please revisit this one?

@jukkar jukkar removed their request for review July 28, 2021 11:11
To turn SMP on pervasively.  Tests should be treated with a combination
of flagging specific cases as "1cpu" where we have short-running tests
that can be independently run in an otherwise SMP environment, and via
setting CONFIG_MP_NUM_CPUS=1 where that's not possible (which still
runs the full SMP kernel config, but with only one CPU available).

Signed-off-by: Watson Zeng <zhiwei@synopsys.com>
@abrodkin abrodkin modified the milestones: v2.7.0, v3.0.0 Sep 21, 2021
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 21, 2022
@cfriedt
Copy link
Member

cfriedt commented Feb 2, 2022

@IRISZZW - needs to be rebased, I think

@ruuddw
Copy link
Member

ruuddw commented Feb 4, 2022

needs to be rebased, I think

Correct, and some of the tests have already been fixed in separate PRs by Anas. Note that IRISZZW moved on and is no longer working on ARC, @evgeniy-paltsev plans to address the remaining test as part of his work on SMP and ARCv3 cores.

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 6, 2022
@cfriedt cfriedt closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARC ARC Architecture area: Kernel area: Networking area: Portability Standard compliant code, toolchain abstraction area: POSIX POSIX API Library area: Samples Samples area: SMP Symmetric multiprocessing area: Sockets Networking sockets area: Tests Issues related to a particular existing or missing test area: Tracing Tracing Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants