Handshake and internal processing for socket groups #1119

ethouris · 2020-02-05T13:08:48Z

No description provided.

…/srt into dev-pre-group-api-complementary

…forced ISN

…-group-api-complementary

…st-handshake

maxsharabayko · 2020-02-10T10:37:46Z

srtcore/api.cpp

      CTimer::triggerEvent();

+      CTimer::triggerEvent();
+


maxsharabayko · 2020-02-10T11:45:42Z

srtcore/buffer.h

+   int capacity() const { return m_iSize; } // XXX WHY it was changed to m_iSize-1 ?



// XXX WHY it was changed to m_iSize-1 ?
This comment was already removed from one of the previous PRs.

Ok, I'll remove this. We'll research during the reimplementation

maxsharabayko · 2020-02-10T11:46:35Z

srtcore/packet.cpp

+          // XXX EXPERIMENTAL. Pass the 32-bit integer here.
+         m_nHeader[SRT_PH_MSGNO] = *lparam;


maxsharabayko · 2020-02-10T11:47:53Z

srtcore/core.cpp

+                int32_t groupdata[GRPD__SIZE];
+                if ( bytelen < GRPD__SIZE * GRPD_FIELD_SIZE)


extra space after the left brace

maxsharabayko · 2020-02-10T11:48:26Z

srtcore/core.cpp

+    //SRTSOCKET master_peerid = groupdata[GRPD_MASTERID];
+    //int32_t tdiff = groupdata[GRPD_MASTERTDIFF];


delete commented code

maxsharabayko · 2020-02-10T11:48:56Z

srtcore/core.cpp

+    /*
+
+    // Synchronize the TSBPD PEER start time with the existing connection,
+    // if there exists the connection with given peer.
+    if (master_peerid != -1)
+    {
+        // Here "I am a peer", so this is the socket ID of local socket of a parallel connection.
+        // Check if it exists, if not, reject the connection.
+        CUDTSocket* master = s_UDTUnited.locateSocket(master_peerid, s_UDTUnited.ERH_RETURN);


Delete commented code

maxsharabayko · 2020-02-10T11:52:32Z

srtcore/core.cpp

+#ifndef SRT_TEST_DISABLE_KEY_CONTROL_PACKETS
+                        sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));
+#endif


This was already removed from one of the previous PRs.

Merging again...

maxsharabayko · 2020-02-10T11:53:05Z

srtcore/core.cpp

+
+    bool need_tsbpd = m_bTsbPd || m_bGroupTsbPd;


const bool need_tsbpd

maxsharabayko · 2020-02-10T11:53:49Z

srtcore/core.cpp

+                // XXX move this code do CUDT::defaultPacketArrival and call it from here:
+
+                // srt_loss_seqs = CALLBACK_CALL(m_cbPacketArrival, rpkt);
+


Not sure whether this comment should stay, but at least no need in extra blank lines

Should stay. This is where the parallel packet lossreport prevention should be added. It's an important improvement for broadcast groups.

maxsharabayko · 2020-02-10T11:54:56Z

srtcore/core.cpp

+    int initial_loss_ttl = 0;
+    if ( self->m_bPeerRexmitFlag )
+        initial_loss_ttl = self->m_iReorderTolerance;


const int initial_loss_ttl = (self->m_bPeerRexmitFlag) ? 0 : self->m_iReorderTolerance;

maxsharabayko · 2020-02-10T11:55:12Z

srtcore/core.cpp

+        int32_t seqlo = CSeqNo::incseq(self->m_iRcvCurrSeqNo);
+        int32_t seqhi = CSeqNo::decseq(pkt.m_iSeqNo);


gou4shi1 · 2022-03-02T16:37:26Z

Hi @ethouris , may I ask you a question about these codes?

                // Do this checking only for groups and only at the very first moment,
                // when there's still nothing in the buffer. Otherwise there will be
                // a serious data discrepancy between the agent and the peer.
                // After increasing by 1, but being previously set as ISN-1, this should be == ISN,
                // if this is the very first packet to send.
#if ENABLE_EXPERIMENTAL_BONDING
                // Fortunately here is only the procedure that verifies if the extraction
                // sequence is moved due to the difference between ISN caught during the existing
                // transmission and the first sequence possible to be used at the first sending
                // instruction. The group itself isn't being accessed.
                if (m_parent->m_GroupOf && m_iSndCurrSeqNo != w_packet.m_iSeqNo && m_iSndCurrSeqNo == m_iISN)
                {
                    const int packetspan = CSeqNo::seqcmp(w_packet.m_iSeqNo, m_iSndCurrSeqNo);

                    HLOGC(qslog.Debug, log << CONID() << "packData: Fixing EXTRACTION sequence " << m_iSndCurrSeqNo
                            << " from SCHEDULING sequence " << w_packet.m_iSeqNo
                            << " DIFF: " << packetspan << " STAMP:" << BufferStamp(w_packet.m_pcData, w_packet.getLength()));

                    // This is the very first packet to be sent; so there's nothing in
                    // the sending buffer yet, and therefore we are in a situation as just
                    // after connection. No packets in the buffer, no packets are sent,
                    // no ACK to be awaited. We can screw up all the variables that are
                    // initialized from ISN just after connection.
                    //
                    // Additionally send the drop request to the peer so that it
                    // won't stupidly request the packets to be retransmitted.
                    // Don't do it if the difference isn't positive or exceeds the threshold.
                    if (packetspan > 0)
                    {
                        int32_t seqpair[2];
                        seqpair[0] = m_iSndCurrSeqNo;
                        seqpair[1] = w_packet.m_iSeqNo;
                        HLOGC(qslog.Debug, log << "... sending INITIAL DROP (ISN FIX): "
                                << "msg=" << MSGNO_SEQ::unwrap(w_packet.m_iMsgNo) << " SEQ:"
                                << seqpair[0] << " - " << seqpair[1] << "(" << packetspan << " packets)");
                        sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));

                        // In case when this message is lost, the peer will still get the
                        // UMSG_DROPREQ message when the agent realizes that the requested
                        // packet are not present in the buffer (preadte the send buffer).
                    }
                }
                else
#endif
                {
                    HLOGC(qslog.Debug, log << CONID() << "packData: Applying EXTRACTION sequence " << m_iSndCurrSeqNo
                            << " over SCHEDULING sequence " << w_packet.m_iSeqNo
                            << " DIFF: " << CSeqNo::seqcmp(m_iSndCurrSeqNo, w_packet.m_iSeqNo)
                            << " STAMP:" << BufferStamp(w_packet.m_pcData, w_packet.getLength()));

#if ENABLE_EXPERIMENTAL_BONDING
                    HLOGC(qslog.Debug, log << "... CONDITION: IN GROUP: " << (m_parent->m_GroupOf ? "yes":"no")
                            << " extraction-seq=" << m_iSndCurrSeqNo << " scheduling-seq=" << w_packet.m_iSeqNo << " ISN=" << m_iISN);
#endif

                    // Do this always when not in a group, 
                    w_packet.m_iSeqNo = m_iSndCurrSeqNo;
                }

The logging says Fixing EXTRACTION sequence m_iSndCurrSeqNo from SCHEDULING sequence w_packet.m_iSeqNo, but there is no m_iSndCurrSeqNo = w_packet.m_iSeqNo; and all following packets to send will be overridden by m_iSndCurrSeqNo which is starting from wrong ISN.
Moreover, it's sending w_packet.m_iSeqNo, why the DROPREQ includes w_packet.m_iSeqNo?

I guess the correct code should be like this?

sendDropReq(m_iSndCurrSeqNo, w_packet.m_iSeqNo - 1);
m_iSndCurrSeqNo = w_packet.m_iSeqNo

Correct me if I'm wrong, thanks!

ethouris · 2022-03-02T17:45:48Z

Ok, I'm not sure what this exactly is without more time spent on the analysis, but what I can see here is that the value w_packet.m_iSeqNo is later overridden with the scheduling sequence, while the value used for dropping is the previous one, which is likely the increased ISN. Note that this overriding and dropping happens only if the scheduling sequence and initial sequence differ.

The problem here was that in UDT the idea was that the packet doesn't get the sequence number at the time when it's being scheduled for sending; this number is given to the packet only at the sending queue. It's not even set when inserting into the queue, it's given to it at the time when the next packet is being picked up for sending. This was a problem for the group implementation that has been created on top of socket sending (let's say, without a strong refax of the code, in this case the sending facilities and the sender buffer, it wasn't possible to do any other way). And as a single packet had to be sent over possibly multiple connections at once, it should have the same sequence number on all member connections. Therefore there had to be one central place where this value has to be controlled, and as it was the group, it must have been decided before you call the single-socket sending procedure. It was expected though that the sequence numbers will roll in both systems the same way, which means that the only way how you can have a different "scheduling sequence" and "internal sequence" was the very first packet sending after activation of an idle link.

gou4shi1 · 2022-03-03T03:29:37Z

@ethouris Thanks for you explanation.

what I can see here is that the value w_packet.m_iSeqNo is later overridden with the scheduling sequence

"extraction sequence" should be m_iSndCurrSeqNo, while "scheduling sequence" should be w_packet.m_iSeqNo which is set in sendmsg2()?
Where is the code of "the value w_packet.m_iSeqNo is later overridden with the scheduling sequence"?

ethouris · 2022-03-03T07:37:46Z

Ups, maybe actuallly I should have spent more time on the analysis ;).

Ok, it's done only as a replacement for previous "increasing the sequence by 1", so not in a situation when these sequences differ. Here the already set sequence is taken as a good deal. Whether the sequence of the packet being sent currently will be effectively dropped, is another matter. At least what I can see is likely that it will be, as dropping uses simply a pair of two sequences and the message is treated as if a packet was received previously with the sequence being the later one.

If you have that case, you should see in the logs the packet sent after this packData call on the receiver side reported as "belated".

gou4shi1 · 2022-03-03T09:23:37Z

If the extraction sequence is differ from scheduling sequence for the first packet, we should override the extraction sequence with scheduling sequence? But I can't see any code that is doing this.

ethouris · 2022-03-03T14:11:26Z

Search for the instruction that assigns to this field. It's about 10 lines down.

gou4shi1 · 2022-03-03T14:32:21Z

Search for the instruction that assigns to this field. It's about 10 lines down.

Did you mean w_packet.m_iSeqNo = m_iSndCurrSeqNo;?
But the log is saying Applying EXTRACTION sequence m_iSndCurrSeqNo over SCHEDULING sequence w_packet.m_iSeqNo,
instead of

If the extraction sequence is differ from scheduling sequence for the first packet, we should override the extraction sequence with scheduling sequence?

i.e. m_iSndCurrSeqNo = w_packet.m_iSeqNo

gou4shi1 · 2022-03-04T15:42:00Z

I guess the correct code should be like this?

diff --git a/srtcore/core.cpp b/srtcore/core.cpp
index 07c4427e..4a906ef1 100644
--- a/srtcore/core.cpp
+++ b/srtcore/core.cpp
@@ -9608,16 +9608,18 @@ bool srt::CUDT::packUniqueData(CPacket& w_packet, time_point& w_origintime)
         {
             int32_t seqpair[2];
             seqpair[0] = m_iSndCurrSeqNo;
-            seqpair[1] = w_packet.m_iSeqNo;
+            seqpair[1] = CSeqNo::decseq(w_packet.m_iSeqNo);
+            const int32_t no_msgno = 0;
             HLOGC(qslog.Debug, log << "... sending INITIAL DROP (ISN FIX): "
                     << "msg=" << MSGNO_SEQ::unwrap(w_packet.m_iMsgNo) << " SEQ:"
                     << seqpair[0] << " - " << seqpair[1] << "(" << packetspan << " packets)");
-            sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));
+            sendCtrl(UMSG_DROPREQ, &no_msgno, seqpair, sizeof(seqpair));
 
             // In case when this message is lost, the peer will still get the
             // UMSG_DROPREQ message when the agent realizes that the requested
             // packet are not present in the buffer (preadte the send buffer).
         }
+        m_iSndCurrSeqNo = w_packet.m_iSeqNo;
     }
     else
 #endif

Search for the instruction that assigns to this field. It's about 10 lines down.

Or did you mean overrideSndSeqNo()? This should only be called in sendBroadcast() and sendBackup()?

ethouris · 2022-03-04T16:23:43Z

Let me try to start over.

The "extraction sequence" (the value written in m_iSndCurrSeqNo) shall be overridden by the scheduling sequence (value written in the packet). In case of a group connection the former can be slightly outdated, although it is believed that it's just few packets behind the scheduling sequence. The scheduling sequence should be the one taking over because this is the value with which this same packet has been sent over another member connection. And all packets before this one should be skipped by the receiver.

The extraction sequence should be used always in case when this is a socket connection, and it gets increased a few lines above.

Whether this upper value of the sequence should be decreased, I'm not sure. It will be then used in CRcvBufferNew::dropMessage and CUDT::dropFromLossLists (on the other side). Basing on what's in CRcvLossList::remove you are probably right, but speak to Max to make sure.

Mikołaj Małecki added 30 commits January 23, 2020 13:41

Complementary changes for socket groups lay-ground

88d5b91

Post self-review fixes

47bf02e

Applied 'unimportant' changes non-group-related from group branch

93eff31

Fixed build break on Windows

2fc45bc

Added lacking new function

abdd38a

Merge branch 'dev-pre-group-api-complementary' of github.com:ethouris…

cd6ed90

…/srt into dev-pre-group-api-complementary

Fixed conflicts

8df4a04

Cosmetix from codefactor

4ab221f

Merge branch 'master' into dev-pre-group-api-complementary

b176b41

Merged epoll changes with post-review fixes

a386c84

Updated from master

ce826cc

Removed unwanted changes

e8ccc01

Withdrawn circbuffer test (moved already to ut)

b5abf42

Fixed problem: forced ISN should be set and recognized as -1 when no …

bdd0070

…forced ISN

Bugfix: use -1 as no-forced-isn

513e7cc

Fixed also in the C API

da87644

Fixed: move tsbpd state setting to correct place

39f29af

Pre-review fixes

8f6ec78

Fixed also in UDT API

934826d

Some formatting fixes

5b5030b

Merge branch 'dev-pre-group-api-complementary-forcedisn' into dev-pre…

313489b

…-group-api-complementary

Merged extracted fixes

90ddbc7

Merge branch 'master' into dev-pre-group-api-complementary

65a0231

Post-review fixes

4ebcb3d

Some new updates from group branch

03266a0

createCond/Mutex renamed to setup

bbc0a33

Partial merge from Haivision#1102

ceef1cf

Updated for changes for the condition variables

171ec06

Updated with changes for Haivision#1102

4eb296c

Updated from Haivision#1102

19bc9dc

ethouris added [core] Area: Changes in SRT library core Impact: High labels Feb 5, 2020

ethouris added this to the v1.5.0 milestone Feb 5, 2020

Mikołaj Małecki added 2 commits February 5, 2020 17:15

Merge branch 'master' into dev-group-all-broadcast-api

eddbef7

Merge branch 'dev-group-all-broadcast-api' into dev-group-all-broadca…

2194166

…st-handshake

Merged with upstream

358c32e

maxsharabayko reviewed Feb 10, 2020

View reviewed changes

srtcore/api.cpp Outdated

Comment on lines 747 to 750

CTimer::triggerEvent();

CTimer::triggerEvent();

Copy link

Collaborator

maxsharabayko Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Fixed wrong automatic merge

ee232b8

maxsharabayko mentioned this pull request Feb 10, 2020

Roadmap: Connection Bonding #1120

Open

33 tasks

maxsharabayko reviewed Feb 10, 2020

View reviewed changes

Mikołaj Małecki added 2 commits February 10, 2020 15:20

Post-review fixes

2ff942c

Comments and cosmetix

7a7838f

maxsharabayko approved these changes Feb 11, 2020

View reviewed changes

maxsharabayko merged commit b41749d into Haivision:master Feb 11, 2020

mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020

gou4shi1 mentioned this pull request Mar 5, 2022

[core] Fix extraction seq with scheduling seq in group. #2258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handshake and internal processing for socket groups #1119

Handshake and internal processing for socket groups #1119

ethouris commented Feb 5, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

ethouris Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

ethouris Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

ethouris Feb 10, 2020

maxsharabayko Feb 10, 2020

maxsharabayko Feb 10, 2020

gou4shi1 commented Mar 2, 2022 •

edited

Loading

ethouris commented Mar 2, 2022

gou4shi1 commented Mar 3, 2022 •

edited

Loading

ethouris commented Mar 3, 2022

gou4shi1 commented Mar 3, 2022 •

edited

Loading

ethouris commented Mar 3, 2022

gou4shi1 commented Mar 3, 2022 •

edited

Loading

gou4shi1 commented Mar 4, 2022 •

edited

Loading

ethouris commented Mar 4, 2022

		int capacity() const { return m_iSize; } // XXX WHY it was changed to m_iSize-1 ?

		// XXX EXPERIMENTAL. Pass the 32-bit integer here.
		m_nHeader[SRT_PH_MSGNO] = *lparam;

		int32_t groupdata[GRPD__SIZE];
		if ( bytelen < GRPD__SIZE * GRPD_FIELD_SIZE)

		//SRTSOCKET master_peerid = groupdata[GRPD_MASTERID];
		//int32_t tdiff = groupdata[GRPD_MASTERTDIFF];

		// XXX move this code do CUDT::defaultPacketArrival and call it from here:

		// srt_loss_seqs = CALLBACK_CALL(m_cbPacketArrival, rpkt);

		int32_t seqlo = CSeqNo::incseq(self->m_iRcvCurrSeqNo);
		int32_t seqhi = CSeqNo::decseq(pkt.m_iSeqNo);

Handshake and internal processing for socket groups #1119

Handshake and internal processing for socket groups #1119

Conversation

ethouris commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gou4shi1 commented Mar 2, 2022 • edited Loading

ethouris commented Mar 2, 2022

gou4shi1 commented Mar 3, 2022 • edited Loading

ethouris commented Mar 3, 2022

gou4shi1 commented Mar 3, 2022 • edited Loading

ethouris commented Mar 3, 2022

gou4shi1 commented Mar 3, 2022 • edited Loading

gou4shi1 commented Mar 4, 2022 • edited Loading

ethouris commented Mar 4, 2022

gou4shi1 commented Mar 2, 2022 •

edited

Loading

gou4shi1 commented Mar 3, 2022 •

edited

Loading

gou4shi1 commented Mar 3, 2022 •

edited

Loading

gou4shi1 commented Mar 3, 2022 •

edited

Loading

gou4shi1 commented Mar 4, 2022 •

edited

Loading