read locking is over used in the kernel to guarantee liveness, this API makes it easy to use EBR to provide liveness guarantees without a global atomic
See D15366 for the initial use case.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/conf/kern.pre.mk | ||
|---|---|---|
| 80 | Maybe OFED_CFLAGS does then no longer need to include -I$S/contrib/ck/include . | |
Comment Actions
Generally I like it.
I'd like it a lot more if there were man.9 page(s) for the epoch API.
Some nits are all I see, but my CK foo is weak.
| sys/conf/kmod.mk | ||
|---|---|---|
| 126 ↗ | (On Diff #42300) | I don't think we need this BOTH places... But if we do, then I'll look into why... this duplication is getting out of hand. |
| sys/kern/subr_epoch.c | ||
| 305 | nit: blank line between functions... | |
| sys/sys/epoch.h | ||
| 4 | Nit: Please consider removing this. It's not needed. | |
| 58 | Nit: Most other files in the tree aren't double-spaced like this. Consider making it single spaced. | |
| sys/sys/proc.h | ||
| 246 | is 255 nesting levels enough? It's hard to tell from the code. | |
| sys/kern/subr_gtaskqueue.c | ||
|---|---|---|
| 992 ↗ | (On Diff #42300) | Did an unrelated change leak in? |
| sys/sys/gtaskqueue.h | ||
| 66 ↗ | (On Diff #42300) | Same question as above: This seems unrelated to the epoch change. |
| sys/sys/proc.h | ||
| 246 | sigqueue_t ends in an int, and td_flags is an int, so it looks like we have 4 bytes between them, one of which is used for td_lend_user_pri. So it seems like there is space for a u_short or uint16_t for this without changing the size of struct thread. Hopefully 64K levels would be enough for anyone. | |
| sys/kern/subr_gtaskqueue.c | ||
|---|---|---|
| 992 ↗ | (On Diff #42300) | It's a dependency. I'll just go and commit it separately today. |
| sys/sys/epoch.h | ||
| 4 | Will do. | |
| sys/sys/proc.h | ||
| 246 | This is morally equivalent to an rlock having a single thread recurse 250 times on an rlock or simultaneously acquire 250 rlocks seems like behaviour that could only be a bug. I'll widen the field if you disagree. | |
Comment Actions
That's reasonable, but I'd like to see everything approved first otherwise it might be wasted effort. Also while you're on the topic, prod cognet to get the ck man pages installed on build. I asked him and he just said "I didn't for some reason".
| sys/kern/subr_epoch.c | ||
|---|---|---|
| 151 | We're incrementing eps right above, the actual ck epoch record is a field in that. | |
Comment Actions
- Forbid recursing in a second epoch
- Replace the ad hoc block handling with turnstile
- Forbid sleeping in an epoch section
Comment Actions
- add stress test
- correctly handle case where thread on wait list is not blocked on a lock yet
- correctly handle case where a thread blocked on a lock is unblocked
- correctly handle case where the memory for a domain is not populated as can be the case if hw.physmem is set much less than the RAM in the system
- add comments explaining the internals of epoch_block_handler
Comment Actions
- Keep stats of paths taken in block handler
- Add adaptive spinning
- remove gratuitous assert
- replace sleep with context switch when there are threads blocked on a turnstile
- Add new testcase variation
- Specify number of iterations as argument to runtest
| sys/kern/subr_epoch.c | ||
|---|---|---|
| 423 | This might be misleading to someone not familiar with LKPI, clarify this as from LinuxKPI? | |
| sys/kern/kern_synch.c | ||
|---|---|---|
| 150 | Probably should do this in sched_bind() too since you rely on sched_pin(). | |
| sys/kern/subr_epoch.c | ||
| 101 | We could probably use a little more of this information included with smp.h or elsewhere. This type of topology information becomes handy as we get smarter about resource placement. | |