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

osq_lock: add define to use smp_cond_load_relaxed. #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matt2909
Copy link

@matt2909 matt2909 commented Oct 8, 2021

Adds a define USE_SMP_COND_LOAD_RELAXED that enables use of
smp_cond_load_relaxed in osq_lock spin (as per v5.6 kernel).

To address issue #69.

Adds a define USE_SMP_COND_LOAD_RELAXED that enables use of
smp_cond_load_relaxed in osq_lock spin (as per v5.6 kernel)
@@ -358,6 +358,20 @@ do { \
} \
VAL; \
})

#define smp_cond_load_relaxed(ptr, cond_expr) \
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -384,6 +398,19 @@ do { \
barrier(); \
VAL; \
})

#define smp_cond_load_relaxed(ptr, cond_expr) ({ \
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -393,7 +393,10 @@ static bool osq_lock(uint64_t *osq, unsigned long cpu_number)
* guaranteed their existence -- this allows us to apply
* cmpxchg in an attempt to undo our queueing.
*/

#if defined(USE_SMP_COND_LOAD_RELAXED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be defined and set as default since it appears to be in the latest linux kernel?

Copy link
Author

@matt2909 matt2909 Oct 8, 2021

Choose a reason for hiding this comment

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

My concern is that someone may be relying on the behaviour of the older kernel code for regression/testing.

If we agree that this isn't a concern and that we want to reflect the latest stable kernel osq_lock then we can change to make this behaviour the default.

Note that we'll also need to update the commentary at the top of osq_lock.h to reflect this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is ok to change the code to reflect what is stable in the kernel. We should leave an option to run the old implementation that can be switched on via a define in the Makefile

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.

2 participants