queueing: generalize DynamicClassifier for pull-based per-class structures

[adamgeorge309]

DynamicClassifier creates the branch of each traffic class the first time a packet of that class arrives. It could build only one shape — a submodule of the configured type in a preexisting submodule vector, wired to a submodule literally named multiplexer — and it built that branch as a side effect of classifying, which is supposed to be a query. This series generalizes it so that it can also build pull-based per-class structures, and fixes the defects that stood in the way.

Everything is confined to DynamicClassifier. PacketClassifierBase and the meaning of classifyPacket() are untouched.

This is the enabling change for the per-station airtime-fair IEEE 802.11 transmit queue, which is proposed separately on top of this branch.

What changes

• Classification is a query. classifyPacket() used to build the branch of a class it had not seen, so classifying changed the model — and every path classifies: the capacity checks classify the packet they are asked about, the pull path classifies on every peek, and delivery classifies again. It is a plain map lookup now. The branch is created where a packet's fate is decided: in pushPacket() and startPacketStreaming(), the two doors of the delivery path, and in canPushPacket(), which creates the branch and asks it — the answer has to hold for the very packet it is asked about, because a source that asks may push exactly that packet next.
• Parametric aggregator. The submodule the branches are wired into is named by the new aggregatorSubmoduleName parameter, still multiplexer by default. 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, so no contract is needed between the classifier and the aggregator beyond wiring the gate.
• Wire first, initialize after. Branch modules were initialized before the branch was connected to the aggregator, and the classifier resolved its sink references while the far end of the path was still incomplete — both traps for a compound branch, the second one leaving a permanently null consumer that bypasses back-pressure. The branch is now initialized once the whole chain is wired, and its packet operations are checked the way the base class checks gates wired in NED.
• Stable class keys. The class-to-branch map was keyed on an index that getOutputGateIndex() maps relative to the current number of output gates, which grows with each branch — so with reverseOrder the same class was looked up under a different key later and got a second branch. The map is keyed on the classifier function's index now, and reverseOrder, which this leaves with nothing to act on, is refused rather than silently ignored.
• Back-pressure. A classifier that has no branch yet used to answer canPushSomePacket() with "no", stopping an active source in front of it before the first branch was ever created, with no notification that could ever restart it. It answers "yes" while it has no branch, and falls back to the inherited answer from the first branch on — that query has no packet, so unlike canPushPacket() it cannot create a branch and ask it, and an active source takes the answer as the licence to produce a packet and push it.

Behavior of existing configurations is unchanged: the defaults reproduce the previous push-multiplexer shape. This is an argument from the defaults, not a measurement — neither in-tree user has a runnable configuration to measure. MacService and PeerService are not instantiated anywhere under examples/, showcases/, tests/ or tutorials/, and tutorials/protocol Network90 stops during network setup on unassigned parameters, on master as much as here. What the tests below do cover is both branch shapes those users have: a simple branch module and a compound one, each wired into a push multiplexer.

Reading order

Eight commits. The first two are behavior-preserving preparation, and are meant to be read with a whitespace-ignoring diff (git show -w) and with --color-moved respectively. The three that follow fix DynamicClassifier in place. Then classification becomes side-effect free, then the back-pressure answer is repaired, and the WHATSNEW entry comes last.

Architectural surface

• No contract changes. PacketClassifierBase, classifyPacket() and every other classifier are untouched; the whole change is DynamicClassifier overriding pushPacket(), startPacketStreaming(), canPushPacket() and canPushSomePacket().
• canPushPacket() is const and creates the branch it is asked about, through a const_cast marked as a kludge, next to the one PacketClassifierBase::callClassifyPacket() already carries. It is the one query whose answer decides the fate of a specific packet, so it is the one query that may build what decides it.
• NED: new aggregatorSubmoduleName parameter with a default; submoduleName and moduleType unchanged; the inherited reverseOrder is refused by DynamicClassifier.
• No new AV-* or NV-* ledger rows, and no sealed path is touched.

Tests

Three queueing tests are added. The module had none.

• DynamicClassifier_1.test — two branches built on demand; an ini file assignment addressing a submodule of a branch takes effect; the statistics of the branch submodules are recorded under the branch path. Its producer is connected to the classifier directly, so it fails without the back-pressure commit.
• DynamicClassifier_2.test — an already created branch is filled up; the producer must stop rather than push into it. Verified to discriminate: reverting the commit makes it fail with Queue is overloaded without a packet dropper.
• DynamicClassifier_3.test — the branches are wired into an aggregator that is not the one named by default.

Commands and results:

$ make MODE=release                      # OK at each of the eight commits
$ inet_run_queueing_tests -m release     # 60 tests, 48 pass, 12 fail

The twelve failures are the ones master fails as well, measured on master in the same
environment: the same twelve test names, in Gate_*, PeriodicGate_1, RedDropper_1,
Tagger_1, OrdinalBased*, TokenBucket* and MultiTokenBucket*. They expect EV log lines
that the current build no longer emits, in modules this branch does not touch.

doc/architecture/enforcement/check-architecture.sh src/inet/queueing reports the same
pre-existing violations as master, and none in the changed files.

Checked separately, outside the test suite: a branch whose first module is a closed
PacketGate — a branch type that refuses its first packet. A PacketServer upstream asks
canPushPacket() and then pushes that exact packet; with the branch created and asked, the
server is told no and keeps the packet, and the run completes. Answering the query without
creating the branch pushes the packet through a gate that is not open.

No fingerprint or statistical baseline is affected: no fingerprint row covers
tutorials/protocol or src/inet/protocolelement, the only existing DynamicClassifier users.

[devin-ai-integration]

Devin Review found 2 potential issues.

View 5 additional findings in Devin Review.

[devin-ai-integration]

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

Suggested change

	parent->setSubmoduleVectorSize(submoduleName, index + 1);
	parent->setSubmoduleVectorSize(submoduleName, std::max(parent->getSubmoduleVectorSize(submoduleName), index + 1));

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[adamgeorge309]

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.

[adamgeorge309]

Fixed in 680d9c8a13 — growSubmoduleVector() takes the max of the current and required size, so a vector declared larger in NED is never truncated.

[adamgeorge309]

Same as on the other thread: the history has been reorganized, so the commit named above is no longer part of the branch. The behavior is unchanged and now lives in createBranchModule(), which calls setSubmoduleVectorSize() with the maximum of the current size and the index it needs, so a vector declared larger in NED is never truncated.

The correction to the premise still stands: setSubmoduleVectorSize() refuses to shrink over a range that still holds submodules rather than deleting them, so the failure mode would have been an aborted run, not modules silently disappearing.

[levy]

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?

[adamgeorge309]

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 POST_MODEL_CHANGE / cPostPathCreateNotification of the connection itself. The classifier no longer knows anything about the aggregator beyond wiring a gate to it.

Two more, from the bot review:

• The PassivePacketSinkRef/ActivePacketSinkRef of a new branch were resolved right after setGateSize(), while the gate was still unconnected. reference() resolves eagerly, so with mandatory=false it stored a nullptr that nothing ever re-resolved: canPushPacket() threw on it, and pushPacket() quietly degraded to send(), bypassing back-pressure. The references are now taken after the branch and the aggregator connection are wired.
• The branch submodule vector was resized to exactly index + 1, which can shrink a vector declared larger in NED. Both resize sites now go through a grow-only helper.

Pushed as three commits on top.

[adamgeorge309]

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:

vector 381 ...channelAccess.pendingQueue.splicetmp.queue queueLength:vector ETV
vector 402 ...channelAccess.pendingQueue.splicetmp.queue queueLength:vector ETV
vector 423 ...channelAccess.pendingQueue.splicetmp.queue queueLength:vector ETV
vector 444 ...channelAccess.pendingQueue.splicetmp.queue queueLength:vector ETV

VectorRecorder::subscribedTo() binds the name once from getComponent()->getFullPath(), and addResultRecorders() runs at the end of cModule::buildInside() — so the path is captured while the module is still a child of splicetmp, before spliceBranch() reparents and renames it into queue[k]/gate[k]. Nothing re-registers afterwards; the handle is only released in the destructor.

Deferring callInitialize() does not help here, since recorders bind strictly earlier, during buildInside(). Scalars are unaffected because they resolve the component at finish() time — which is why the .sca correctly shows queue[0..3]/gate[0..3] while the .vec does not.

A fix needs each branch module to have its final parent and name before buildInside() runs on it, which the throwaway-compound approach cannot give. I have not attempted it yet.

[adamgeorge309]

Pushed two commits.

21fc255974 records the statistics of spliced branch submodules under their final module path. They are built inside the temporary branch compound, and an output vector keeps the full path it had when it was registered, so every branch recorded its vectors under the same ...splicetmp.<name> path -- indistinguishable from each other, and per-statistic configuration was matched against that path as well. The result recorders are now recreated once the submodule is in its final place. Scalars were never affected, being recorded at finish().

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. **.splicetmp.**.vector-record-empty = false removes those; the NED documentation says so.

643a5dd07f adds tests/queueing/DynamicClassifier_1.test, which builds two branches and checks the recorded module paths. It fails on the parent commit.

Two related defects I did not touch here:

• An ini assignment that targets a spliced submodule by its final path is silently ignored, because parameters are finalized under the temporary name too: **.pendingQueue.queue[*].packetCapacity = 5 has no effect, while **.splicetmp.queue.packetCapacity = 5 does. Unlike the recorders, this cannot be repaired after the fact -- parameter finalization is one-shot, and by then a NED assignment can no longer be told from a default, so ini-versus-NED precedence is unreconstructible. Configuration has to go through the enclosing queue's parameters, which forwardMatchingParams() copies into the branch compound by name.
• A classifier that has no branch yet reports canPushSomePacket() == false, so an ActivePacketSource stops before the first branch is ever created. The test puts a BackPressureBarrier in front to work around it.

[adamgeorge309]

2ad8345198 fixes the second defect from the previous comment: a classifier that has no branch yet answered canPushSomePacket() == false, so an ActivePacketSource in front of it stopped and waited for a notification that nothing was going to send -- no branch, no packet, no branch. It now answers true, since a packet of an unseen class is taken by the branch created for it.

The inherited canPushPacket() was the worse half: it classifies the packet, and here classifying creates the branch, so a query built submodules, grew gate vectors, wired connections and initialized modules. It now looks the class up and only delegates to a branch that already exists. The pull side classifies in canPullPacket() as well and is deliberately left alone -- there the query is what drives branch creation, and this classifier has no pull user.

One detail worth flagging: the class index is now taken straight from the classifier function instead of going through PacketClassifier::classifyPacket(). That applies the reverseOrder mapping, which is relative to the current number of output gates -- and that number grows with every branch, so the same class would have been looked up under a different key later and given a second branch.

The module test no longer needs the BackPressureBarrier that was hiding this, so it covers both defects now: without the fix the producer never produces and no branch is ever built.

[levy]

I think dynamic packet classification doesn't belong to the PacketClassifierBase, it belongs to DynamicClassifier.

why do we add to PacketClassifierBase and change the meaning of classifyPacket?

virtual int createGateForPacket(Packet *packet);
virtual bool canCreateGateForPacket(Packet *packet) const;

why do we need to change? what do they have to do with dynamic packet classification?

BehaviorAggregateClassifier::canPushPacket
MultiFieldClassifier::canPushPacket

[adamgeorge309]

Why it went into the base

The starting point was the earlier note that classifyPacket() has to be free of side effects. That means branch creation has to move to the delivery path, and the delivery path lives in the base: pushPacket() and startPacketStreaming() are the only two places that know a packet is being delivered rather than asked about. createGateForPacket() is called from exactly those two. canCreateGateForPacket() exists because canPushPacket() then has to answer for a packet whose gate does not exist yet, and it must not create one in order to find out.

On changing the meaning of classifyPacket()

The -1 is not new. On master PacketClassifier::classifyPacket() already passes it through on purpose:

return index == -1 ? index : getOutputGateIndex(index);

and four classifiers already produce it: PriorityClassifier when every consumer is full, WrrClassifier when every consumer is empty, and BehaviorAggregateClassifier / MultiFieldClassifier when nothing matches. callClassifyPacket() throws on it today. So the PR changes how an existing value is interpreted rather than introducing one. That does not make the change necessary — it only means "no existing output gate suits this packet" is already what those four mean by -1.

You are right that it can stay in DynamicClassifier

It comes to three overrides, because startPacketStreaming() is the single funnel for all three streaming entry points (pushPacketStart, pushPacketEnd, pushPacketProgress all go through it):

void DynamicClassifier::pushPacket(Packet *packet, const cGate *gate);          // create the branch, then delegate
void DynamicClassifier::startPacketStreaming(Packet *packet);                   // the same, on the streaming path
bool DynamicClassifier::canPushPacket(Packet *packet, const cGate *gate) const; // look up only, never create

classifyPacket() is then a plain map lookup that never returns -1, because the branch already exists by the time the base classifies. PacketClassifierBase and the meaning of classifyPacket() stay untouched, and the two new virtuals disappear.

What it costs is that DynamicClassifier has to know which methods of the base are the delivery path, and silently misses a new one if the base ever grows one. That was the trade in the other direction, and it does not outweigh leaving the contract alone.

BehaviorAggregateClassifier::canPushPacket and MultiFieldClassifier::canPushPacket

Nothing — they have nothing to do with dynamic classification, and they are only there as collateral of the base change. Both return -1 for a packet that matches nothing and send it out of defaultOut in their own pushPacket(). Once -1 stops being an error in the inherited canPushPacket(), they answer "I cannot take this packet" about a packet they would in fact take. On master the same query throws instead, so neither answer is right, but the base change would have replaced a loud wrong answer with a silent one. If the base change goes, that commit goes with it.

Proposal

Move it into DynamicClassifier as above and drop both the base-class commit and the diffserv commit. That takes the series from ten commits to eight. Classifier_3.test goes with them, since it tests the base's refusal; the three DynamicClassifier tests stay, including the one that pins the back-pressure answer. The WHATSNEW entry loses its paragraph about the classifier contract and keeps the one about the module.

[adamgeorge309]

Done — the change is now confined to DynamicClassifier. PacketClassifierBase and both diffserv classifiers are byte-identical to master again, createGateForPacket() and canCreateGateForPacket() are gone, and classifyPacket() keeps its meaning. Ten commits down to eight.

The branch is created in the three places that decide a packet's fate: pushPacket() and startPacketStreaming() (the two doors of the delivery path — the three streaming push operations all classify through the latter), and canPushPacket().

canPushPacket() creates the branch and asks it, rather than answering by lookup. Your point about the contract is right, and it is not theoretical. With a branch whose first module is a closed PacketGate and a PacketServer upstream, which asks canPushPacket(packet) and then pushes that same packet:

• answering yes by lookup, without the branch: <!> Error: Illegal operation on the gate, a packet is being passed through while the gate is not open -- ... branch[0].gate, at t=0s, event #1
• creating the branch and asking it: the server is told no, keeps the packet, and the run completes.

The cost is a const_cast in a const query, marked as a kludge next to the one callClassifyPacket() already carries, and a branch built for a class that is asked about but may never receive a packet.

canPushSomePacket() cannot be made exact the same way: it has no packet, so it has no class, and there is nothing to create and ask. For a classifier with no branch, true is a guess and false is a deadlock. It answers true only while no branch exists and defers to the inherited answer afterwards. This is the pre-existing weakness of the query rather than a new one — PacketClassifierBase::canPushSomePacket() already returns true when any consumer has room, and an active source then produces a packet that may classify to a full one.

Verified: all eight commits build; inet_run_queueing_tests -m release is 60 tests, 48 pass, 12 fail, the same twelve master fails in this environment.