Page MenuHomeFreeBSD
Differential D30604

libalias: Restructure - Remove temporary state deleteAllLinks from global struct
ClosedPublic
Actions

Authored by donner on Jun 1 2021, 6:31 PM.
Tags
None
Referenced Files
F112852279: D30604.id90850.diff
Sun, Mar 23, 2:31 PM
Unknown Object (File)
Sun, Feb 23, 3:47 PM
Unknown Object (File)
Jan 26 2025, 2:13 PM
Unknown Object (File)
Jan 26 2025, 2:12 PM
Unknown Object (File)
Jan 25 2025, 4:20 PM
Unknown Object (File)
Jan 23 2025, 12:45 AM
Unknown Object (File)
Jan 13 2025, 6:58 PM
Unknown Object (File)
Jan 10 2025, 12:46 PM
View All Files
Subscribers
imp
melifaro
Contributor Reviews (src)

Details

Reviewers
None
Group Reviewers
network
Commits
rGfe83900f9fa9: libalias: Restructure - Remove temporary state deleteAllLinks from global struct
Summary

The entry deleteAllLinks in the struct libalias is only used to signal
a state between internal calls. It's not used between API calls.

Test Plan

Use testsuite.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

donner created this revision.Jun 1 2021, 6:31 PM
Herald added subscribers: Contributor Reviews (src), melifaro, imp. · View Herald TranscriptJun 1 2021, 6:31 PM
donner requested review of this revision.Jun 1 2021, 6:31 PM
donner added a child revision: D30582: libalias: Restructure - Finalize.Jun 1 2021, 6:32 PM
Harbormaster completed remote builds in B39652: Diff 90282.Jun 1 2021, 6:32 PM
donner added a reviewer: network.Jun 1 2021, 6:32 PM
donner updated this revision to Diff 90843.Jun 13 2021, 8:51 PM

rebase

Harbormaster completed remote builds in B39892: Diff 90843.Jun 13 2021, 8:51 PM
donner updated this revision to Diff 90850.Jun 13 2021, 9:22 PM
  • Remove an unnecessary local var
Harbormaster completed remote builds in B39899: Diff 90850.Jun 13 2021, 9:22 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 19 2021, 7:58 PM
Closed by commit rGfe83900f9fa9: libalias: Restructure - Remove temporary state deleteAllLinks from global struct (authored by donner). · Explain Why
This revision was automatically updated to reflect the committed changes.
donner added a commit: rGfe83900f9fa9: libalias: Restructure - Remove temporary state deleteAllLinks from global struct.
donner mentioned this in rG78d515b2225e: libalias: Restructure.Jul 6 2021, 6:57 AM
donner mentioned this in rGe0a40dc22771: libalias: Restructure.Jul 6 2021, 7:12 AM

Revision Contents
Changeset List

PathSize
sys/
netinet/
libalias/
58 lines
3 lines

Diff 91120

View Options

sys/netinet/libalias/alias_db.c

Show First 20 Lines • Show All 1,046 Lines • ▼ Show 20 Lines while (work_done < budget) {
tx->dqo.compl_head = (tx->dqo.compl_head + 1) & tx->dqo.compl_head = (tx->dqo.compl_head + 1) &
tx->dqo.compl_mask; tx->dqo.compl_mask;
/* Flip the generation bit when we wrap around */ /* Flip the generation bit when we wrap around */
tx->dqo.cur_gen_bit ^= tx->dqo.compl_head == 0; tx->dqo.cur_gen_bit ^= tx->dqo.compl_head == 0;
work_done++; work_done++;
} }
/*
* Waking the xmit taskqueue has to occur after room has been made in
* the queue.
*/
atomic_thread_fence_seq_cst();
if (atomic_load_bool(&tx->stopped) && work_done) {
atomic_store_bool(&tx->stopped, false);
markjUnsubmitted
Done
Inline Actions

Suppose we load stopped, and then are preempted. Then, the transmit path tries to enqueue a packet and fails with ENOBUFS, so it sets stopped = 1. The only way we can make progress is by running the cleanup task again, right?

I think this mechanism works, but I'm wondering if we really need a full barrier here. It looks like acquire/release loads/stores of stopped would be sufficient.

markj: Suppose we load `stopped`, and then are preempted. Then, the transmit path tries to enqueue a…
shailend_google.comAuthorUnsubmitted
Done
Inline Actions

Suppose we load stopped, and then are preempted. Then, the transmit path tries to enqueue a packet and fails with ENOBUFS, so it sets stopped = 1. The only way we can make progress is by running the cleanup task again, right?

Yup. The correctness relies on the ordering between the space made by this thread in the ring and its load of stopped.

If this thread loads a 0 and is preempted and then the xmit thread ENOBUFSs, that implies that the space this thread made before getting preempted wasn't enough to xmit a pkt, which means there is a future interrupt coming that will eventually make space.

Note that the xmit thread does not ever schedule this cleanup thread, only the hardware summons it to work by irq.

I think this mechanism works, but I'm wondering if we really need a full barrier here. It looks like acquire/release loads/stores of stopped would be sufficient.

Since I need to load stopped after making the space in the ring, am I not forced to use a barrier: there being no "release" variant of load?

shailend_google.com: > Suppose we load `stopped`, and then are preempted. Then, the transmit path tries to enqueue a…
markjUnsubmitted
Done
Inline Actions

We do also have atomic_thread_fence_*, to be used instead of mb() etc. when needed. In particular, atomic_thread_fence_seq_cst() should be equivalent to mb() on all platforms.

markj: We do also have atomic_thread_fence_*, to be used instead of mb() etc. when needed. In…
taskqueue_enqueue(tx->xmit_tq, &tx->xmit_task);
}
tx->done += work_done; /* tx->done is just a sysctl counter */ tx->done += work_done; /* tx->done is just a sysctl counter */
counter_enter(); counter_enter();
counter_u64_add_protected(tx->stats.tbytes, bytes_done); counter_u64_add_protected(tx->stats.tbytes, bytes_done);
counter_u64_add_protected(tx->stats.tpackets, pkts_done); counter_u64_add_protected(tx->stats.tpackets, pkts_done);
counter_exit(); counter_exit();
return (work_done == budget); return (work_done == budget);
} }
Show All 18 Lines
View Options

sys/netinet/libalias/alias_local.h

Show First 20 Lines • Show All 1,046 Lines • ▼ Show 20 Lines while (work_done < budget) {
tx->dqo.compl_head = (tx->dqo.compl_head + 1) & tx->dqo.compl_head = (tx->dqo.compl_head + 1) &
tx->dqo.compl_mask; tx->dqo.compl_mask;
/* Flip the generation bit when we wrap around */ /* Flip the generation bit when we wrap around */
tx->dqo.cur_gen_bit ^= tx->dqo.compl_head == 0; tx->dqo.cur_gen_bit ^= tx->dqo.compl_head == 0;
work_done++; work_done++;
} }
/*
* Waking the xmit taskqueue has to occur after room has been made in
* the queue.
*/
atomic_thread_fence_seq_cst();
if (atomic_load_bool(&tx->stopped) && work_done) {
atomic_store_bool(&tx->stopped, false);
markjUnsubmitted
Done
Inline Actions

Suppose we load stopped, and then are preempted. Then, the transmit path tries to enqueue a packet and fails with ENOBUFS, so it sets stopped = 1. The only way we can make progress is by running the cleanup task again, right?

I think this mechanism works, but I'm wondering if we really need a full barrier here. It looks like acquire/release loads/stores of stopped would be sufficient.

markj: Suppose we load `stopped`, and then are preempted. Then, the transmit path tries to enqueue a…
shailend_google.comAuthorUnsubmitted
Done
Inline Actions

Suppose we load stopped, and then are preempted. Then, the transmit path tries to enqueue a packet and fails with ENOBUFS, so it sets stopped = 1. The only way we can make progress is by running the cleanup task again, right?

Yup. The correctness relies on the ordering between the space made by this thread in the ring and its load of stopped.

If this thread loads a 0 and is preempted and then the xmit thread ENOBUFSs, that implies that the space this thread made before getting preempted wasn't enough to xmit a pkt, which means there is a future interrupt coming that will eventually make space.

Note that the xmit thread does not ever schedule this cleanup thread, only the hardware summons it to work by irq.

I think this mechanism works, but I'm wondering if we really need a full barrier here. It looks like acquire/release loads/stores of stopped would be sufficient.

Since I need to load stopped after making the space in the ring, am I not forced to use a barrier: there being no "release" variant of load?

shailend_google.com: > Suppose we load `stopped`, and then are preempted. Then, the transmit path tries to enqueue a…
markjUnsubmitted
Done
Inline Actions

We do also have atomic_thread_fence_*, to be used instead of mb() etc. when needed. In particular, atomic_thread_fence_seq_cst() should be equivalent to mb() on all platforms.

markj: We do also have atomic_thread_fence_*, to be used instead of mb() etc. when needed. In…
taskqueue_enqueue(tx->xmit_tq, &tx->xmit_task);
}
tx->done += work_done; /* tx->done is just a sysctl counter */ tx->done += work_done; /* tx->done is just a sysctl counter */
counter_enter(); counter_enter();
counter_u64_add_protected(tx->stats.tbytes, bytes_done); counter_u64_add_protected(tx->stats.tbytes, bytes_done);
counter_u64_add_protected(tx->stats.tpackets, pkts_done); counter_u64_add_protected(tx->stats.tpackets, pkts_done);
counter_exit(); counter_exit();
return (work_done == budget); return (work_done == budget);
} }
Show All 18 Lines