Details

Reviewers

kib
markj
gallatin

Commits

rS364381: MFC r364019:
rS364019: Add full support support for dynamic allocation and freeing of epoch's.

Summary

Make sure to reclaim epoch structures when they are freed to support dynamic allocation and freeing of epoch's.

While at it, move the 64 supported epoch control structures, to the static memory domain.
This overall simplifies the management and debugging of system EPOCH's.

Sponsored by: Mellanox Technologies
MFC after: 1 week

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• hselasky created this revision.Aug 5 2020, 3:18 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 5 2020, 3:18 PM

• hselasky requested review of this revision.Aug 5 2020, 3:18 PM

Harbormaster completed remote builds in B32772: Diff 75430.Aug 5 2020, 3:18 PM

• hselasky retitled this revision from Make sure to reclaim NULL elements in allepoch array to Make sure to reclaim NULL elements in allepochs array.Aug 5 2020, 3:18 PM

• hselasky edited the summary of this revision. (Show Details)

markj accepted this revision.Aug 5 2020, 3:31 PM

markj added inline comments.

sys/kern/subr_epoch.c
328 ↗	(On Diff #75430)	Can't we just use a global `sx` to synchronize epoch allocation and free?

This revision is now accepted and ready to land.Aug 5 2020, 3:31 PM

We could use atomics. I don't know if sx's are ready when we allocate epochs.

In D25960#575501, @hselasky wrote:

We could use atomics. I don't know if sx's are ready when we allocate epochs.

SI_SUB_EPOCH is long after SI_SUB_LOCK.

kib added inline comments.Aug 5 2020, 6:51 PM

sys/kern/subr_epoch.c
355 ↗	(On Diff #75430)	May be we should bite the bullet and return NULL if there is no space in the array ?

See updated summary.

This revision now requires review to proceed.Aug 6 2020, 9:54 AM

Harbormaster completed remote builds in B32796: Diff 75476.Aug 6 2020, 9:54 AM

No more need for e_idx field.

Harbormaster completed remote builds in B32797: Diff 75477.Aug 6 2020, 9:55 AM

• hselasky edited the summary of this revision. (Show Details)Aug 6 2020, 9:59 AM

• hselasky added a reviewer: gallatin.

Assert valid name before freeing epoch.

Harbormaster completed remote builds in B32798: Diff 75478.Aug 6 2020, 10:02 AM

adding Drew, in case he sees something which is not performancewise clever

kib added inline comments.Aug 6 2020, 10:06 AM

sys/kern/subr_epoch.c
342 ↗	(On Diff #75478)	I do not quite understand how this loop works. You only loop up to the top allocated index, not to MAX_EPOCH, so if there is no holes, you cannot allocate anything.

Address comment by @kib. The increment of "epoch_count" was accidentially removed by patch update.

Harbormaster completed remote builds in B32799: Diff 75481.Aug 6 2020, 11:01 AM

• hselasky marked an inline comment as done.Aug 6 2020, 11:02 AM

• hselasky added inline comments.

sys/kern/subr_epoch.c
342 ↗	(On Diff #75478)	See the epoch_count++ below.

• hselasky retitled this revision from Make sure to reclaim NULL elements in allepochs array to Add support full support for dynamic allocation and freeing of epochs.Aug 6 2020, 11:05 AM

• hselasky edited the summary of this revision. (Show Details)

• hselasky retitled this revision from Add support full support for dynamic allocation and freeing of epochs to Add full support support for dynamic allocation and freeing of epochs.

kib added inline comments.Aug 6 2020, 12:46 PM

sys/kern/subr_epoch.c
363 ↗	(On Diff #75481)	I suggest renaming this variable into something like epoch_array_max_slot. It is not epoch_count.

Rename epoch_count to epoch_array_max_count, as suggested by kib@

Harbormaster completed remote builds in B32806: Diff 75491.Aug 6 2020, 1:15 PM

kib added inline comments.Aug 6 2020, 1:19 PM

sys/kern/subr_epoch.c
336 ↗	(On Diff #75491)	This blank line is not needed.
341 ↗	(On Diff #75491)	Comments should be a proper sentences.
737 ↗	(On Diff #75491)	Is it safe to iterate to epoch_array_max_count instead of MAX_EPOCHS ? Could we miss an update to max_count there and then skip some epoch ?

Address comments from @kib

Harbormaster completed remote builds in B32808: Diff 75496.Aug 6 2020, 2:00 PM

• hselasky added inline comments.Aug 6 2020, 2:02 PM

sys/kern/subr_epoch.c
737 ↗	(On Diff #75491)	e_name is protected by global_epoch.

kib accepted this revision.Aug 6 2020, 2:18 PM

kib added inline comments.

sys/kern/subr_epoch.c
737 ↗	(On Diff #75491)	From what I see, global epoch ensures that e_name is not cleared until epoch_call_task() ends it global section. I asked about different situation, where the update from epoch_alloc (most likely, to epoch_array_max_count) is missed by epoch_call_task and then corresponding epoch is not drained by the task. But I see that you changed the loop limit to MAX_EPOCHS already.

This revision is now accepted and ready to land.Aug 6 2020, 2:18 PM

markj added inline comments.Aug 6 2020, 2:20 PM

sys/kern/subr_epoch.c
367 ↗	(On Diff #75496)	Please use atomic(9) instead of `volatile`.

Use atomic(9) as suggested by @markj

This revision now requires review to proceed.Aug 6 2020, 2:56 PM

Harbormaster completed remote builds in B32809: Diff 75502.Aug 6 2020, 2:56 PM

markj added inline comments.Aug 6 2020, 3:59 PM

sys/kern/subr_epoch.c
86 ↗	(On Diff #75502)	Why is it defined like this? I can't see the purpose of `val`.

• hselasky added inline comments.Aug 6 2020, 4:03 PM

sys/kern/subr_epoch.c
86 ↗	(On Diff #75502)	The atomic functions don't accept "const char *ptr", instead of casting it in the code, I make a cast in the union ...

markj accepted this revision.Aug 7 2020, 1:52 PM

markj added inline comments.

sys/kern/subr_epoch.c
86 ↗	(On Diff #75502)	I don't object but personally I'd prefer the ugliness of the casts to the union. atomic_(load\|store)_ptr were changed to not require casts recently, it's only the MD atomic_(load\|store)_(acq\|rel)_* macros that still need it for now. Please add a comment explaining why it's a union.