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

Priorities can overflow for plausible values of MT #56

Open
abouteiller opened this issue Jun 10, 2022 · 3 comments
Open

Priorities can overflow for plausible values of MT #56

abouteiller opened this issue Jun 10, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@abouteiller
Copy link
Contributor

abouteiller commented Jun 10, 2022

Describe the bug

Priorities can overflow for plausible values of MT (e.g., running with M=500k, MB=180 results in MT >1500, which would overflow the priorities an cause scheduling to try to run in the 'wrong' order).

Additional context

#53 (comment)

Possible fixes

  1. priorities could be 'float' type, but that requires a change we do not really like on the PaRSEC side.
  2. We need to somehow clamp the priorities back into an int32 at the end, but we can use any intermediate representation to avoid overflow in the meantime. Question is how we do that without crushing all priorities for the tail-end of the execution?
  3. We need to define priority classes that are somewhat independent of MT^3, maybe having independent task priorities for each individual GEMM is overkill, and having priority per-panel/update iteration is enough
@abouteiller abouteiller added the enhancement New feature or request label Jun 10, 2022
@omor1
Copy link
Contributor

omor1 commented Aug 11, 2022

Is it possible to use a 64-bit priority instead of 32-bit? That allows MT up to 2–2.6M, which should be more than sufficient. That still requires annoying changes on the PaRSEC side, but maybe not as invasive as for a floating-point priority.

@bosilca
Copy link
Contributor

bosilca commented Aug 11, 2022

It is always possible, but the task_s is actually quite packed and tightly aligned. Increasing the size of the priority field to an int64_t will increase the size of the parsec_task_s struct by 8 bytes, and push a lot of things into the next cache line.

That being said, we had a parallel discussion about improving the chores mask to be able to limit the migration opportunities for a task. We should be able to combine these two ideas and use the extra space provided by increasing the size of the priority field for the chose_mask field (which is currently an uint8_t and could be transitioned to an uint32_t).

@abouteiller
Copy link
Contributor Author

ICLDisco/parsec#706

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

No branches or pull requests

3 participants