[core] Fixed usages of m_iRcvLastAck.

[gou4shi1]

As discussed in #2500 (comment).

[ethouris]

Is this function that retrieves the buffer size being called without a lock on the buffer, or under a lock? I'm kinda confused here.

[gou4shi1]

Is this function that retrieves the buffer size being called without a lock on the buffer, or under a lock? I'm kinda confused here.

There are two functions:

size_t srt::CUDT::getAvailRcvBufferSizeLock() const
{
    ScopedLock lck(m_RcvBufferLock);
    return getAvailRcvBufferSizeNoLock();
}

size_t srt::CUDT::getAvailRcvBufferSizeNoLock() const
{
    return m_pRcvBuffer->getAvailSize(m_iRcvLastSkipAck);
}

[codecov-commenter]

Codecov Report

Merging #2530 (1f91905) into master (cbfa812) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2530      +/-   ##
==========================================
+ Coverage   64.82%   65.24%   +0.41%     
==========================================
  Files         100      100              
  Lines       19767    19767              
==========================================
+ Hits        12814    12896      +82     
+ Misses       6953     6871      -82     

Impacted Files	Coverage Δ
srtcore/core.h	72.15% <ø> (ø)
srtcore/core.cpp	59.88% <100.00%> (+0.54%)	⬆️
srtcore/buffer_snd.cpp	67.10% <0.00%> (-0.66%)	⬇️
test/test_bonding.cpp	93.68% <0.00%> (-0.53%)	⬇️
srtcore/api.cpp	53.28% <0.00%> (+0.05%)	⬆️
srtcore/queue.cpp	81.34% <0.00%> (+0.74%)	⬆️
srtcore/window.cpp	83.11% <0.00%> (+1.29%)	⬆️
srtcore/congctl.cpp	80.82% <0.00%> (+2.07%)	⬆️
srtcore/list.cpp	83.19% <0.00%> (+13.82%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ethouris]

Ok. We use markers for such functions, sometimes only an informative comment

// [[using locked(m_RcvBufferLock)]]

I can see also that there is a marker for static thread checker, that should also explain things.

[ethouris]

@maxsharabayko What exactly was the reason to call it this way?

I'm analyzing the contents of this function (getAvailSize) and - as long as the start sequence in the buffer is properly synchronized with the events of dropping (TSBPD) and acknowledging (ACK handler), both calls, the before fix and after fix will simply resolve to calling m_pRcvBuffer->capacity().

This is because, if that's true about the synchronization, m_iRcvLastSkipAck is always equal to m_pRcvBuffer->getStartSeqNo() and the m_iRcvLastAck is either equal to it or in the past towards it, that is, values from both these fields always satisfy this condition:

        if (CSeqNo::seqcmp(iRBufSeqNo, iFirstUnackSeqNo) >= 0) // iRBufSeqNo >= iFirstUnackSeqNo

The call to this function with passing it the sequence number may make sense only if you are passing a sequence number that is potentially within the range of the receiver buffer, and may only unexpectedly be in the past. This way you get the number of packets that you can still add to the buffer, but then the sequence number that you should pass there should be the sequence number of the last received packet, not last ACKNOWLEDGED packet. If this is to be the value to be reported via the ACKD_BUFFERLEFT field in the ACK report, it should be this exactly - the distance between the last received packet and the end of buffer because this value is an information for the sender side whether it is allowed to send anything, or maybe it should stop because the packet will be dropped due to no possibility to store it in the receiver buffer. That's maybe not exactly necessary for the live mode (you can't stop sending in live mode anyway, and impossibility to store received packets should result in either large drop or breaking the connection), but it is important for the file mode.

[maxsharabayko]

The name comes from the old implementation of the receiver buffer to have a consistent mapping from old receiver buffer to the new one.
Now that the old receiver buffer is completely removed, we can fully use the new receiver buffer logic.
I am looking at the CUDT::handleSocketPacketReception(..) and see that the return value of the m_pRcvBuffer->insert(u) is not used at the total power. E.g. the following condition can likely be replaced just with checking if the m_pRcvBuffer->insert(u) returned -3:

const int avail_bufsize = (int) getAvailRcvBufferSizeNoLock();
if (offset >= avail_bufsize)

[ethouris]

Well, I forgot one important detail: the old receiver buffer had one tricky part: there could be a range that was already acknowledged, but not yet extracted by the application. This one theoretically has it as well, but there is no limitation as to what packets the API is allowed to read. The problem is that the buffer contains the following ranges:

-x-x-x-x-x-0-0-x-x-0-x-x-x-x-0...

The first 5 packets: initial contiguous range, then there are two lost packets, still followed by some valid packets. There is total 14 packets in the buffer.

The question is then, what value we want to present to the sender peer as a "number of free space in the receiver buffer". The capacity decreased by 5 or 14?

[maxsharabayko]

Capacity minus ACKed packets that are still in the buffer.

[gou4shi1]

Replaced by #2535