Page MenuHomeFreeBSD

Add full support support for dynamic allocation and freeing of epochs
ClosedPublic

Authored by hselasky on Aug 5 2020, 3:18 PM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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 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.

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.

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 ?

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

See updated summary.

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

No more need for e_idx field.

hselasky added a reviewer: gallatin.

Assert valid name before freeing epoch.

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

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.

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.
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.

hselasky marked an inline comment as done.

Rename epoch_count to epoch_array_max_count, as suggested by kib@

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 ?

hselasky marked 6 inline comments as done.

Address comments from @kib

sys/kern/subr_epoch.c
737 ↗(On Diff #75491)

e_name is protected by global_epoch.

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
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
sys/kern/subr_epoch.c
86 ↗(On Diff #75502)

Why is it defined like this? I can't see the purpose of val.

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 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.

This revision is now accepted and ready to land.Aug 7 2020, 1:52 PM
This revision now requires review to proceed.Aug 7 2020, 3:20 PM
This revision is now accepted and ready to land.Aug 7 2020, 3:22 PM