search

inet-framework / inet

INET Framework for the OMNeT++ discrete event simulator
https://inet.omnetpp.org
Other
442 stars 488 forks source link

Fix ATS token statistic #888

Closed Musteblume closed 1 year ago

Musteblume commented 1 year ago

Emit numTokens in EligibilityTimeMeter was negative when alpha < 0 (bucketEmptyTime is future event). The number of Tokens should never be negativ.

levy commented 1 year ago

Thanks for the PR! Somehow the diff shows that all lines are changed! Is it the line endings or what?

Musteblume commented 1 year ago

Oh, this could be the case -.-

Musteblume commented 1 year ago

Fixed that^^

levy commented 1 year ago

would be nice to have them squashed, otherwise I can't see what is really changed

Musteblume commented 1 year ago

Done

levy commented 1 year ago

Could you please add a bit more reasoning why the new code is better than the old one? I thought that the number of credits is a virtual measure anyway, so being negative is not such a big concern. I remember we talked about this briefly once and we came to the conclusion that having negative values may not be a problem after all. If we don't allow negative values (e.g. clamp at 0) than there's no way of seeing what happens with the virtual number of tokens in that period.

Musteblume commented 1 year ago

I had some cases where that presentation has confused people. I also think in alpha < 0 cases the presentation is wrong. The virtual number of tokens just jump into negative space because of the sign.

The new presentation should be correct and depict the behavior of the virtual tokens in those cases. When the bucketEmptyTime is a future point in time: The packet is the one clearing the bucket. So, the previous token number are the number of bits of this packet. Also, i dont think there is a clamp at zero because it is just the calculated bucketEmptyTime that is emited. At this point in time, the bucket is empty.

What do you think?

Best regards

levy commented 1 year ago

I'm still not sure if I understand right what is expected and what is changed by this PR. Could you please attach an image that shows the difference between the old/new behavior for the number of tokens diagram?

As a side note, the automated fingerprint test for this pull request stopped for ATS with the following error:

/showcases/tsn/trafficshaping/asynchronousshaper/ -f omnetpp.ini -c General -r 0  (runtime error with exitcode=1: <!> Error: Signal 'tokensChanged' emitted as timestamped value with wrong data type (expected=double, actual=int) -- in module (inet::EligibilityTimeMeter) TsnLinearNetwork.switch.bridging.streamFilter.ingress.meter[0] (id=202), at t=0.011515177461s, event #1613

Maybe this has been fixed since then, because I don't see where the code emits an integer???

BTW, looking at the code, the number of tokens statistic doesn't affect the ATS behavior, it's just a derived statistic. I would still like to fully understand the suggested change and the reasoning behind before pushing this PR upstream.

We can also arrange a video meeting if that's better in your opinion. That would allow you to show me the examples where this is issue pops up.

Musteblume commented 1 year ago

Yes, lets arrange a video meeting i will contact you via mail.

levy commented 1 year ago

This PR has been merged in manually after the reasoning and implications were discussed in a video call.