Drop fewer packets on UDP packet flood (#5565)

* ACK received-but-unprocessed packets when queued

* Prioritize packets between high- and low-priority (high-priority are: reliable packets that need an ACK)

* Increase max size of LLPacketRing buffer

7 files changed, 67 insertions, 57 deletions

diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 79752d2315..7f06f991b0 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -5894,7 +5894,6 @@ void LLAppViewer::idleNetwork()
     pingMainloopTimeout("idleNetwork");
 
     gObjectList.mNumNewObjects = 0;
-    S32 total_decoded = 0;
 
     static LLCachedControl<bool> speed_test(gSavedSettings, "SpeedTest", false);
     if (!speed_test())
@@ -5902,64 +5901,56 @@ void LLAppViewer::idleNetwork()
         LL_PROFILE_ZONE_NAMED_CATEGORY_NETWORK("idle network"); //LL_RECORD_BLOCK_TIME(FTM_IDLE_NETWORK); // decode
 
         LLTimer check_message_timer;
-        //  Read all available packets from network
         const S64 frame_count = gFrameCount;  // U32->S64
-        F32 total_time = 0.0f;
+        S32 total_decoded = 0;
 
+        // Process packets from network
+        LockMessageChecker lmc(gMessageSystem);
+        while (lmc.checkAllMessages(frame_count, gServicePump))
         {
-            bool needs_drain = false;
-            LockMessageChecker lmc(gMessageSystem);
-            while (lmc.checkAllMessages(frame_count, gServicePump))
-            {
-                if (gDoDisconnect)
-                {
-                    // We're disconnecting, don't process any more messages from the server
-                    // We're usually disconnecting due to either network corruption or a
-                    // server going down, so this is OK.
-                    break;
-                }
-
-                total_decoded++;
+            ++total_decoded;
 
-                if (total_decoded > MESSAGE_MAX_PER_FRAME)
+            // Time-box processing of network packets to prevent framerate catastrophe
+            if (check_message_timer.getElapsedTimeF32() >= CheckMessagesMaxTime)
+            {
+                // Drain the socket buffer so we know how many messages remain to process
+                S32 num_buffered_packets = gMessageSystem->drainUdpSocket();
+                if (num_buffered_packets > total_decoded)
                 {
-                    needs_drain = true;
-                    break;
+                    // Grow CheckMessagesMaxTime until we process more packets each frame than arrive.
+                    // This might spiral out of control on very slow computers on fast networks when
+                    // the bandwidth settings are too high.  There is a mechanism for providing backpressure
+                    // to network bandwidth but it may be inadequate for the task
+                    // (see LLViewerThrottle::updateDynamicThrottle() for more details).
+                    CheckMessagesMaxTime *= 1.035f; // 3.5% ~= 2x in 20 frames, ~8x in 60 frames
                 }
-
-                // Prevent slow packets from completely destroying the frame rate.
-                // This usually happens due to clumps of avatars taking huge amount
-                // of network processing time (which needs to be fixed, but this is
-                // a good limit anyway).
-                total_time = check_message_timer.getElapsedTimeF32();
-                if (total_time >= CheckMessagesMaxTime)
+                else if (num_buffered_packets == 0)
                 {
-                    needs_drain = true;
-                    break;
+                    // Reset CheckMessagesMaxTime to default value
+                    CheckMessagesMaxTime = CHECK_MESSAGES_DEFAULT_MAX_TIME;
                 }
+                break;
             }
-            if (needs_drain || gMessageSystem->mPacketRing.getNumBufferedPackets() > 0)
+
+            if (total_decoded > MESSAGE_MAX_PER_FRAME)
             {
-                // Rather than allow packets to silently backup on the socket
-                // we drain them into our own buffer so we know how many exist.
-                S32 num_buffered_packets = gMessageSystem->drainUdpSocket();
-                if (num_buffered_packets > 0)
-                {
-                    // Increase CheckMessagesMaxTime so that we will eventually catch up
-                    CheckMessagesMaxTime *= 1.035f; // 3.5% ~= 2x in 20 frames, ~8x in 60 frames
-                }
+                // MESSAGE_MAX_PER_FRAME is very high (400)
+                // We expect to run out of time before reaching here, but just in case...
+                gMessageSystem->drainUdpSocket();
+                break;
             }
-            else
+
+            if (gDoDisconnect)
             {
-                // Reset CheckMessagesMaxTime to default value
-                CheckMessagesMaxTime = CHECK_MESSAGES_DEFAULT_MAX_TIME;
+                // We're disconnecting so no need to process packets.
+                break;
             }
+        }
 
-            // Handle per-frame message system processing.
+        // Handle per-frame message system processing.
 
-            static LLCachedControl<F32> ack_collection_time(gSavedSettings, "AckCollectTime", 0.1f);
-            lmc.processAcks(ack_collection_time());
-        }
+        static LLCachedControl<F32> ack_collection_time(gSavedSettings, "AckCollectTime", 0.1f);
+        lmc.processAcks(ack_collection_time());
     }
     add(LLStatViewer::NUM_NEW_OBJECTS, gObjectList.mNumNewObjects);
 
diff --git a/indra/newview/llstartup.cpp b/indra/newview/llstartup.cpp
index 6a85ca069a..cd4c9d40b1 100644
--- a/indra/newview/llstartup.cpp
+++ b/indra/newview/llstartup.cpp
@@ -401,7 +401,7 @@ void do_startup_frame()
                 break;
             }
         }
-        if (needs_drain || gMessageSystem->mPacketRing.getNumBufferedPackets() > 0)
+        if (needs_drain || gMessageSystem->getNumBufferedPackets() > 0)
         {
              gMessageSystem->drainUdpSocket();
         }
@@ -785,7 +785,7 @@ bool idle_startup()
 
 
             F32 dropPercent = gSavedSettings.getF32("PacketDropPercentage");
-            msg->mPacketRing.setDropPercentage(dropPercent);
+            msg->setDropPercentage(dropPercent);
         }
 
         LL_INFOS("AppInit") << "Message System Initialized." << LL_ENDL;
diff --git a/indra/newview/llviewermenu.cpp b/indra/newview/llviewermenu.cpp
index ff111dd1bc..7f36244685 100644
--- a/indra/newview/llviewermenu.cpp
+++ b/indra/newview/llviewermenu.cpp
@@ -2298,7 +2298,7 @@ class LLAdvancedDropPacket : public view_listener_t
 {
     bool handleEvent(const LLSD& userdata)
     {
-        gMessageSystem->mPacketRing.dropPackets(1);
+        gMessageSystem->dropPackets(1);
         return true;
     }
 };
diff --git a/indra/newview/llviewerstats.cpp b/indra/newview/llviewerstats.cpp
index 6490fe76a2..3b2796572b 100644
--- a/indra/newview/llviewerstats.cpp
+++ b/indra/newview/llviewerstats.cpp
@@ -791,7 +791,7 @@ void send_viewer_stats(bool include_preferences)
     LLSD &fail = body["stats"]["failures"];
 
     fail["send_packet"] = (S32) gMessageSystem->mSendPacketFailureCount;
-    fail["dropped"] = (S32) gMessageSystem->mDroppedPackets;
+    fail["dropped"] = (S32) gMessageSystem->getTotalNumDroppedPackets();
     fail["resent"] = (S32) gMessageSystem->mResentPackets;
     fail["failed_resends"] = (S32) gMessageSystem->mFailedResendPackets;
     fail["off_circuit"] = (S32) gMessageSystem->mOffCircuitPackets;
diff --git a/indra/newview/llviewerthrottle.cpp b/indra/newview/llviewerthrottle.cpp
index 3ccfbea6e2..90a60c7a94 100644
--- a/indra/newview/llviewerthrottle.cpp
+++ b/indra/newview/llviewerthrottle.cpp
@@ -45,8 +45,8 @@ using namespace LLOldEvents;
 const F32 MAX_FRACTIONAL = 1.5f;
 const F32 MIN_FRACTIONAL = 0.2f;
 
-const F32 MIN_BANDWIDTH = 50.f;
-const F32 MAX_BANDWIDTH = 6000.f;
+const F32 MIN_BANDWIDTH = 50.f;   // Kbps
+const F32 MAX_BANDWIDTH = 6000.f; // Kbps
 const F32 STEP_FRACTIONAL = 0.1f;
 const F32 HIGH_BUFFER_LOAD_TRESHOLD = 1.f;
 const F32 LOW_BUFFER_LOAD_TRESHOLD = 0.8f;
@@ -244,6 +244,8 @@ void LLViewerThrottle::sendToSim() const
 
 F32 LLViewerThrottle::getMaxBandwidthKbps()
 {
+    // Why are thse different than the constants with the same names higher up?
+    // Anybody know? -- Leviathan
     constexpr F32 MIN_BANDWIDTH = 100.0f; // 100 Kbps
     constexpr F32 MAX_BANDWIDTH = 10000.0f; // 10 Mbps
 
@@ -309,6 +311,21 @@ void LLViewerThrottle::resetDynamicThrottle()
 
 void LLViewerThrottle::updateDynamicThrottle()
 {
+    // User configurable bandwidth settings are merely vague aspirations.  Translating those
+    // to what should be sent to the servers is complicated.  Servers will tend to spike data
+    // transmission upon arrival, packets will arrive faster than we can process them, and this
+    // can overflow the buffer, which causes packet loss.
+    //
+    // In an attempt to avoid catastrophe we periodically measure buffer load and packet loss
+    // and then transmit a modified desired bandwidth to the server.  Unfortunately, this system
+    // does not work well for spikey data bursts because:
+    //   (1) the response takes too long (up to 5 seconds) to kick in
+    //   (2) once it starts it tends to ramp up too slowly
+    //   (3) it doesn't know which region is providing the flood; it just assumes it is
+    //       all from the main region
+    //
+    // TODO: fix those ^^^ problems
+
     if (mUpdateTimer.getElapsedTimeF32() < DYNAMIC_UPDATE_DURATION)
     {
         return;
@@ -347,5 +364,7 @@ void LLViewerThrottle::updateDynamicThrottle()
         LL_INFOS() << "Easing network throttle to " << mCurrentBandwidth << LL_ENDL;
     }
 
+    // Now that we've used mBufferLoadRate we reset it to zero because otherwise it only
+    // increases (see clamping behavior in setBufferLoadRate()).
     mBufferLoadRate = 0;
 }
diff --git a/indra/newview/llviewerthrottle.h b/indra/newview/llviewerthrottle.h
index ef898a97d7..7ff0a7d1f0 100644
--- a/indra/newview/llviewerthrottle.h
+++ b/indra/newview/llviewerthrottle.h
@@ -77,8 +77,8 @@ public:
 
     static const std::string sNames[TC_EOF];
 protected:
-    F32 mMaxBandwidth;
-    F32 mCurrentBandwidth;
+    F32 mMaxBandwidth;      // bps
+    F32 mCurrentBandwidth;  // bps
     F32 mBufferLoadRate = 0;
 
     LLViewerThrottleGroup mCurrent;
diff --git a/indra/newview/llworld.cpp b/indra/newview/llworld.cpp
index d02694de7d..cad36828d1 100644
--- a/indra/newview/llworld.cpp
+++ b/indra/newview/llworld.cpp
@@ -794,10 +794,10 @@ void LLWorld::updateNetStats()
 
     S32 packets_in = gMessageSystem->mPacketsIn - mLastPacketsIn;
     S32 packets_out = gMessageSystem->mPacketsOut - mLastPacketsOut;
-    S32 packets_lost = gMessageSystem->mDroppedPackets - mLastPacketsLost;
+    S32 packets_lost = gMessageSystem->mLostPackets - mLastPacketsLost;
 
-    F64Bits actual_in_bits(gMessageSystem->mPacketRing.getAndResetActualInBits());
-    F64Bits actual_out_bits(gMessageSystem->mPacketRing.getAndResetActualOutBits());
+    F64Bits actual_in_bits(gMessageSystem->getAndResetActualInBits());
+    F64Bits actual_out_bits(gMessageSystem->getAndResetActualOutBits());
 
     add(LLStatViewer::MESSAGE_SYSTEM_DATA_IN, actual_in_bits);
     add(LLStatViewer::MESSAGE_SYSTEM_DATA_OUT, actual_out_bits);
@@ -815,7 +815,7 @@ void LLWorld::updateNetStats()
 
     mLastPacketsIn = gMessageSystem->mPacketsIn;
     mLastPacketsOut = gMessageSystem->mPacketsOut;
-    mLastPacketsLost = gMessageSystem->mDroppedPackets;
+    mLastPacketsLost = gMessageSystem->mLostPackets;
 }
 
 
@@ -838,7 +838,7 @@ void LLWorld::printPacketsLost()
                     << " packets lost: " << cdp->getPacketsLost() << LL_ENDL;
         }
     }
-    LL_INFOS() << "Packets dropped by Packet Ring: " << gMessageSystem->mPacketRing.getNumDroppedPackets() << LL_ENDL;
+    LL_INFOS() << "Packets dropped by Packet Ring: " << gMessageSystem->getTotalNumDroppedPackets() << LL_ENDL;
 }
 
 void LLWorld::processCoarseUpdate(LLMessageSystem* msg, void** user_data)