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.

Sign up for GitHub

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
Open

osq_lock: add define to use smp_cond_load_relaxed. #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions ext/linux/include/lk_atomics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

change coming from https://elixir.bootlin.com/linux/v5.14.10/source/arch/arm64/include/asm/barrier.h#L159

({ \
typeof(ptr) __PTR = (ptr); \
typeof(*ptr) VAL; \
for (;;) { \
VAL = READ_ONCE(*__PTR); \
if (cond_expr) \
break; \
__cmpwait_relaxed(__PTR, VAL); \
} \
VAL; \
})

#else
#define __smp_store_release(p, v) \
do { \
Expand All @@ -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.

change coming from https://elixir.bootlin.com/linux/v5.14.10/source/include/asm-generic/barrier.h#L214

typeof(ptr) __PTR = (ptr); \
typeof(*ptr) VAL; \
for (;;) { \
VAL = READ_ONCE(*__PTR); \
if (cond_expr) \
break; \
cpu_relax(); \
} \
VAL; \
})

#endif

#define arch_mcs_spin_lock_contended(l) \
Expand Down
6 changes: 5 additions & 1 deletion ext/linux/osq_lock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

if (smp_cond_load_relaxed(&node->locked, VAL || (++back_off > unqueue_retry)))
return true;
#else
while (!READ_ONCE(node->locked)) {
/*
* TODO: Need to better emulate kernel rescheduling in user space.
Expand All @@ -414,6 +417,7 @@ static bool osq_lock(uint64_t *osq, unsigned long cpu_number)
cpu_relax();
}
return true;
#endif

unqueue:
/*
Expand Down