queueing: generalize DynamicClassifier for pull-based per-class structures - #1122
queueing: generalize DynamicClassifier for pull-based per-class structures#1122adamgeorge309 wants to merge 8 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| outputGates.push_back(classifierOutputGate); | ||
| PassivePacketSinkRef consumer; | ||
| consumer.reference(classifierOutputGate, false); | ||
| consumers.push_back(consumer); | ||
| ActivePacketSinkRef collector; | ||
| collector.reference(classifierOutputGate, false); | ||
| collectors.push_back(collector); |
There was a problem hiding this comment.
🔴 Newly created traffic-class branches are never linked to the classifier, so packets of a new class break delivery
The downstream target of a newly added output is looked up (consumer.reference(...) at src/inet/queueing/classifier/DynamicClassifier.cc:56) before the branch is actually attached to that output, so the classifier ends up with no known destination for that class.
Impact: The first packet of every new traffic class either aborts the simulation with a null-reference error or silently falls back to a different delivery path, breaking normal flow control.
Reference resolution happens on a still-unconnected gate
ModuleRefByGate::reference(gate, false) (src/inet/common/ModuleRefByGate.h:80-89) resolves the peer immediately by walking gate->getNextGate() (src/inet/common/ModuleAccess.h:126-134). At DynamicClassifier.cc:55-60 the new out[index] gate has just been created by setGateSize() and is not connected yet -- the connection is only made later inside createModuleBranch() (src/inet/queueing/classifier/DynamicClassifier.cc:98) or spliceBranch() (src/inet/queueing/classifier/DynamicClassifier.cc:140). With mandatory == false the lookup silently yields nullptr, and the references are never re-resolved.
Consequences in PacketClassifierBase:
canPushSomePacket()/canPushPacket()(src/inet/queueing/base/PacketClassifierBase.cc:86-98) call intoconsumers[i], whosecheckReference()throws"Dereferencing nullptr...".pushPacket()usespushOrSendPacket()(src/inet/queueing/base/PacketProcessorBase.cc:118-126), which falls back tosend()when the consumer is null, bypassing the synchronous push API and back-pressure.
The previous implementation connected the gate first and only then created the reference, so it resolved correctly. The fix is to move the outputGates/consumers/collectors bookkeeping after the branch (and aggregator) wiring is done.
Prompt for agents
In DynamicClassifier::createBranch() (src/inet/queueing/classifier/DynamicClassifier.cc), the PassivePacketSinkRef and ActivePacketSinkRef for the new out[index] gate are resolved via reference(gate, false) immediately after setGateSize(), while the gate is still unconnected. ModuleRefByGate::reference() resolves the peer eagerly by following the connection, so with mandatory==false both refs become nullptr permanently. The gate is only connected later, in createModuleBranch()/spliceBranch(). This makes consumers[index] null, which makes PacketClassifierBase::canPushPacket()/canPushSomePacket() throw and makes pushPacket() fall back to send() instead of the push API. Restructure createBranch() so that outputGates/consumers/collectors are populated only after the branch has been created and classifierOutputGate has been connected to the branch input (the previous implementation connected first, then referenced). Note outputGates must stay index-aligned with consumers/collectors.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f03585e — the references are now taken after the branch and the aggregator connection are wired, so reference() resolves against a connected path instead of storing a silent nullptr.
There was a problem hiding this comment.
Fixed in f03585e590 — the sink references are now taken after the branch is wired through to the aggregator. They resolve the far end of the path eagerly, so taking them earlier resolved them against a gate that was not connected yet.
| cGate *DynamicClassifier::createModuleBranch(int index, cGate *classifierOutputGate, std::vector<cModule *>& modulesToInitialize) | ||
| { | ||
| cModule *parent = getParentModule(); | ||
| parent->setSubmoduleVectorSize(submoduleName, index + 1); |
There was a problem hiding this comment.
🟡 Creating the first per-class branch can delete pre-existing branch modules declared in the network description
The branch container is resized to exactly the new branch position (setSubmoduleVectorSize(submoduleName, index + 1) at src/inet/queueing/classifier/DynamicClassifier.cc:96) instead of only ever growing it, so any pre-existing branches beyond that position are destroyed.
Impact: Statically configured per-class branches can silently disappear at runtime, so traffic that should flow through them is lost or the run aborts.
Removal of the std::max() guard
The previous code deliberately used parentModule->setSubmoduleVectorSize(submoduleName, std::max(origVectorSize, submoduleIndex + 1)) so the vector was never shrunk. The new code passes index + 1 unconditionally. index is the classifier's current out gate count, which is not necessarily >= the NED-declared vector size (e.g. a parent declaring defragmenter[numDefragmenter] whose classifier out gate vector was sized independently). Shrinking an existing submodule vector deletes the elements above the new size. The same unguarded resize is repeated in the splice path at src/inet/queueing/classifier/DynamicClassifier.cc:133.
| parent->setSubmoduleVectorSize(submoduleName, index + 1); | |
| parent->setSubmoduleVectorSize(submoduleName, std::max(parent->getSubmoduleVectorSize(submoduleName), index + 1)); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 680d9c8, via a grow-only helper at both resize sites. One correction to the premise: setSubmoduleVectorSize() refuses to shrink over a range that still holds submodules rather than deleting them, so the failure mode was an aborted run, not modules silently disappearing.
There was a problem hiding this comment.
Fixed in 680d9c8a13 — growSubmoduleVector() takes the max of the current and required size, so a vector declared larger in NED is never truncated.
|
The IDynamicInputScheduler interface has no implementors, how does this work? What's the point of having this interface? Why doesn't the module use the signals emitted when a gate gets connected? |
|
Both fair points — fixed. The interface is gone. Its only implementor lived in the follow-up airtime-fairness branch, so within this PR it was an orphan contract. And the notification is the better mechanism: an aggregator that needs to notice a runtime-added input now picks it up from the Two more, from the bot review:
Pushed as three commits on top. |
|
A separate defect in the splice path, not covered by any thread above. Spliced branches record all their vectors under the temporary compound's path, so per-station vectors are indistinguishable. In a 4-station run, all four sub-queues emit the same vector name — four ids, one name:
Deferring A fix needs each branch module to have its final parent and name before |
|
Pushed two commits.
One residue: a vector is declared in the result file when it is registered, so the discarded recorders leave an empty declaration behind under the temporary name.
Two related defects I did not touch here:
|
|
The inherited One detail worth flagging: the class index is now taken straight from the classifier function instead of going through The module test no longer needs the |
2ad8345 to
47ba427
Compare
…ssifyPacket Invert the class lookup into an early return, so that the block creating the branch of a first-seen class sits at function level instead of inside the conditional. Whitespace-only except for the inverted condition and the hoisted return -- review with a whitespace-ignoring diff. No change in behavior. This puts the block in position for the next commit to move it out verbatim.
Extract-function move: the block that builds a branch -- grows the submodule vector, creates the module, wires it between the classifier and the multiplexer, and initializes it -- becomes createBranch(), the lines byte-identical (review with --color-moved). The class-to-branch map entry stays at the call site, fed by the return value: the map is classification bookkeeping, and createBranch() is topology only. No change in behavior. classifyPacket() reads as what it is: look the class up, create its branch on first sight.
~DynamicClassifier could only wire a branch into a submodule literally named "multiplexer". The downstream aggregator is now named by the aggregatorSubmoduleName parameter (still "multiplexer" by default), and it may be a pull scheduler instead of a push multiplexer: an aggregator that has to take notice of an input appearing at runtime learns about it from the POST_MODEL_CHANGE notification of the connection being made (cPostPathCreateNotification), so no contract is needed between the classifier and the aggregator beyond wiring the gate. For the pull side the classifier now also takes a collector reference per branch, the way it already took a consumer reference for the push side. The missing-submodule-vector and missing-aggregator cases fail with a clear error naming the module instead of a null dereference.
… wired Branch modules were initialized right after being built, before the branch was connected to the aggregator, and the classifier took its sink references on its new out gate while the far end of the path was still incomplete. Both are traps for a compound branch: a module that resolves its downstream peer in initialize() would see a dangling gate, and ModuleRefByGate::reference() resolves the peer eagerly by walking the connection -- with mandatory=false it silently stores a nullptr that nothing ever re-resolves, leaving a permanently null consumer whose canPushPacket() throws and whose pushPacket() quietly degrades to send(), bypassing back-pressure. Wire first, resolve and initialize after: createModuleBranch() builds the branch module (with its final name and index, so its parameters, display string and result recording are all resolved for the module path it keeps) and defers its initialization; createBranch() connects the chain up to and including the aggregator, then takes the references and runs the deferred initializations. No change in behavior for the existing simple-branch users, where the old order happened to be safe.
… indices The class-to-branch map was keyed on the result of PacketClassifier::classifyPacket(), which maps the classifier function's index through getOutputGateIndex(). With reverseOrder that mapping is relative to the current number of output gates -- which grows with each branch created -- so the same class would be looked up under a different key later, miss, and get a second branch. Key the map on the classifier function's index directly, taken through the new getClassIndex(), which classifies without the branch-creating side effect of classifyPacket().
canPushSomePacket() is inherited as "one of the existing branches can take a packet", which is false for a classifier that has not built any branch yet. An active source in front of such a classifier stops, waits for the notification that would tell it packets can be pushed again, and never gets it, because nothing else creates the first branch. Answer true instead: a packet of a class that has not been seen yet is taken by the branch created for it, and the range of the classifier function is not known here, so there may always be such a class. canPushPacket() is worse than useless in its inherited form here: it classifies the packet, and for this classifier classifying creates the branch of a new class, so a query that is supposed to be a query builds submodules, grows gate vectors, wires connections and initializes the new modules. Look the class up instead, and only delegate to the branch that already exists. (The pull side classifies in canPullPacket() too, and is left alone: there the query is what drives branch creation, and this classifier has no pull user.) The module test covers this -- the producer is connected to the classifier directly, so without the fix it never produces and no branch is built -- along with the rest of the contract: two branches built on demand, an ini file assignment addressing a submodule of a branch taking effect, and the statistics of the branch submodules being recorded under the branch path.
47ba427 to
ad7b15b
Compare
…classification classifyPacket() is a query: the capacity checks (canPushPacket(), canPullPacket()) classify the very packet whose delivery classifies it again, and the pull path classifies it on every peek, so classification must be free of side effects. The contract is now spelled out, together with the convention -- already followed by PriorityClassifier, WrrClassifier and the diffserv classifiers -- that -1 means "no existing output gate suits this packet" rather than an out-of-range error. What happens to such a packet is decided on the delivery path only: pushPacket() and startPacketStreaming() ask the new createGateForPacket() hook, whose default refuses the packet just as the range check did, and canPushPacket() asks its side-effect-free query pair, canCreateGateForPacket(). A classifier that extends itself on demand creates its new output gate in the hook, never from a query. The pull-side callers pass -1 through unchanged: canPullPacket() already answers "not for this gate", the pullPacket() family already refuses, and handleCanPullPacketChanged() now skips the notification instead of indexing collectors[-1]. The stateful classifyPacket() implementations that remain (WrrClassifier, TokenBucketClassifier, MultiTokenBucketClassifier) are out of scope here; they are why callClassifyPacket() keeps its const_cast kludge.
classifyPacket() now only answers where a packet of an already-seen class goes, and -1 for a class that has no branch yet; the branch is created in createGateForPacket(), which the base class calls on packet delivery only. The canPushPacket() override disappears: the inherited implementation, with canCreateGateForPacket() answering that a branch can be created for every class, gives the same answers -- an unseen class is always welcome, a seen one asks its branch.
2 participants
DynamicClassifiercould only build one shape: create a submodule of the configured type in a preexisting submodule vector, and wire it to a submodule literally namedmultiplexer. This generalizes it along three axes so it can also build pull-based per-class structures.aggregatorSubmoduleName(stillmultiplexerby default). If it implements the newIDynamicInputSchedulercontract, the runtime-created input gate is registered with it — so the aggregator can be a pull scheduler instead of a push multiplexer.spliceBranchSubmodules, a compoundmoduleTypeforming a linearin -> a -> b -> ... -> outchain is flattened: its inner submodules are reparented into the classifier's parent as vector elements named after them (a[k],b[k]). A downstream matched-pair scheduler can then address them directly, which a compound boundary would prevent.forwardMatchingParams()copies same-named parameters from the enclosing module into the created branch before it is finalized.Branch module initialization is deferred until the whole chain — including the aggregator connection — is wired, since a module that resolves its downstream peer in
initialize()would otherwise see a dangling gate.submoduleNamebecomes optional (unused when splicing), and the missing-vector / missing-aggregator cases now fail with a clear error instead of a null dereference.Behaviour of existing configurations is unchanged: the defaults reproduce the previous push-multiplexer shape.
This is the enabling change for the per-station airtime-fair 802.11 transmit queue, which is proposed separately on top of this branch.
Test
Builds at every commit. Existing
DynamicClassifierusers are unaffected by construction (defaults unchanged).🤖 Generated with Claude Code