Merge remote-tracking branch 'origin/topic/awelzel/pkt-src-get-next-timeout-rework'

* origin/topic/awelzel/pkt-src-get-next-timeout-rework:
  Allow offline packet sources to register FDs.
  PktSrc: Avoid calling ExtractNextPacketInternal() in GetNextTimeout()

CHANGES

@@ -1,3 +1,34 @@
+6.0.0-dev.189 | 2023-03-13 09:55:56 +0100
+
+  * Allow offline packet sources to register FDs. (Jan Grashoefer, Corelight)
+
+  * GH-2039: GH-2842: PktSrc: Avoid calling ExtractNextPacketInternal() in GetNextTimeout() (Arne Welzel, Corelight)
+
+    This reworks 2aec7640dd60c3ea3cffd82461588567e406db34 (zeek/zeek#2039) to
+    avoid calling ExtractNextPacketInternal() within GetNextTimeout() for
+    the non-pseudo-realtime case. Also relates to zeek/zeek#2842.
+
+    The intention of the referenced change was to avoid a 0.00002 timeout when
+    a non-selectable packet source has more packets queued. This was implemented
+    by checking for a new packet within GetNextTimeout().
+
+    The proposed change switches to an predictive approach: Use the result of
+    the previous ExtractNextPacket() call (stored as had_packet) as an indication
+    whether more packets are to be expected.
+
+    Calling ExtractNextPacketInternal() within GetNextTimeout() may cause
+    surprising behavior as some packet source may block [1] or spent a significant
+    amount of time (e.g. applying BPF filters [2]) within ExtractNextPacket().
+    The result of GetNextTimeout() should be available immediately as guidance
+    for the main-loop and the actual work should happen within the ->Process()
+    method.
+
+    This change also attempts to separate the pseudo-realtime logic from the
+    non-pseudo-realtime in an attempt show pseudo-realtime as special.
+
+    [1] https://github.com/hosom/bro-napatech/blob/00c4d657e034927301d5f8d9bc03eca81e619699/src/Napatech.cc#L116
+    [2] https://github.com/sethhall/bro-myricom/blob/58b25c8ebac6d184ff8ff0c27f8da2b603694dab/src/Myricom.cc#L250
+
 6.0.0-dev.185 | 2023-03-13 09:38:34 +0100
 
   * GH-2837: cirrus: Add smoke testing for builtin plugins (Arne Welzel, Corelight)

VERSION

@@ -1 +1 @@
-6.0.0-dev.185
+6.0.0-dev.189

src/iosource/PktSrc.cc

@@ -30,6 +30,8 @@ PktSrc::Properties::Properties()
 PktSrc::PktSrc()
 	{
 	have_packet = false;
+	// Small lie to make a new PktSrc look like the previous ExtractNextPacket() was successful.
+	had_packet = true;
 	errbuf = "";
 	SetClosed(true);
 	}
@@ -83,17 +85,11 @@ void PktSrc::Opened(const Properties& arg_props)
 		}
 
 	if ( props.is_live )
-		{
 		Info(util::fmt("listening on %s\n", props.path.c_str()));
 
-		// We only register the file descriptor if we're in live
-		// mode because libpcap's file descriptor for trace files
-		// isn't a reliable way to know whether we actually have
-		// data to read.
 	if ( props.selectable_fd != -1 )
 		if ( ! iosource_mgr->RegisterFd(props.selectable_fd, this) )
 			reporter->FatalError("Failed to register pktsrc fd with iosource_mgr");
-		}
 
 	DBG_LOG(DBG_PKTIO, "Opened source %s", props.path.c_str());
 	}
@@ -102,7 +98,7 @@ void PktSrc::Closed()
 	{
 	SetClosed(true);
 
-	if ( props.is_live && props.selectable_fd != -1 )
+	if ( props.selectable_fd != -1 )
 		iosource_mgr->UnregisterFd(props.selectable_fd, this);
 
 	DBG_LOG(DBG_PKTIO, "Closed source %s", props.path.c_str());
@@ -179,6 +175,8 @@ bool PktSrc::ExtractNextPacketInternal()
 
 	if ( ExtractNextPacket(&current_packet) )
 		{
+		had_packet = true;
+
 		if ( current_packet.time < 0 )
 			{
 			Weird("negative_packet_timestamp", &current_packet);
@@ -191,6 +189,10 @@ bool PktSrc::ExtractNextPacketInternal()
 		have_packet = true;
 		return true;
 		}
+	else
+		{
+		had_packet = false;
+		}
 
 	if ( run_state::pseudo_realtime && ! IsOpen() )
 		{
@@ -276,33 +278,14 @@ bool PktSrc::GetCurrentPacket(const Packet** pkt)
 
 double PktSrc::GetNextTimeout()
 	{
-	bool pkt_available = have_packet;
-
-	if ( props.selectable_fd == -1 || run_state::pseudo_realtime )
-		pkt_available = ExtractNextPacketInternal();
-
 	if ( run_state::is_processing_suspended() )
 		return -1;
 
-	// If there's no file descriptor for the source, which is the case for some interfaces like
-	// myricom, we can't rely on the polling mechanism to wait for data to be available. As gross
-	// as it is, just spin with a short timeout here so that it will continually poll the
-	// interface. The old IOSource code had a 20 microsecond timeout between calls to select()
-	// so just use that.
-	if ( props.selectable_fd == -1 )
+	// If we're in pseudo-realtime mode, find the next time that a packet is ready
+	// and have poll block until then.
+	if ( run_state::pseudo_realtime )
 		{
-		if ( ! pkt_available && ! run_state::pseudo_realtime )
-			return 0.00002;
-		}
-	// If we're live we want poll to do what it has to with the file descriptor. If we're not live
-	// but we're not in pseudo-realtime mode, let the loop just spin as fast as it can. If we're
-	// in pseudo-realtime mode, find the next time that a packet is ready and have poll block until
-	// then.
-	else if ( IsLive() )
-		return -1;
-
-	if ( ! run_state::pseudo_realtime )
-		return 0;
+		ExtractNextPacketInternal();
 
 		// This duplicates the calculation used in run_state::check_pseudo_time().
 		double pseudo_time = current_packet.time - run_state::detail::first_timestamp;
@@ -311,4 +294,25 @@ double PktSrc::GetNextTimeout()
 		return std::max(0.0, pseudo_time - ct);
 		}
 
+	// If there's no file descriptor for the source, which is the case for some interfaces
+	// like myricom, we can't rely on the polling mechanism to wait for data to be
+	// available. As gross as it is, just spin with a short timeout here so that it will
+	// continually poll the interface. The old IOSource code had a 20 microsecond timeout
+	// between calls to select() so just use that.
+
+	// A heuristic to avoid short sleeps when a non-selectable packet source has more
+	// packets queued is to return 0.0 if the source has yielded a packet on the
+	// last call to ExtractNextPacket().
+	if ( props.selectable_fd == -1 )
+		{
+		if ( have_packet || had_packet )
+			return 0.0;
+
+		return 0.00002;
+		}
+
+	// If there's an FD (offline or live) we want poll to do what it has to with it.
+	return -1.0;
+	}
+
 	} // namespace zeek::iosource

src/iosource/PktSrc.h

@@ -362,6 +362,8 @@ private:
 
 	bool have_packet;
 	Packet current_packet;
+	// Did the previous call to ExtractNextPacket() yield a packet.
+	bool had_packet;
 
 	// For BPF filtering support.
 	std::vector<detail::BPF_Program*> filters;

src/iosource/pcap/Source.cc

@@ -182,10 +182,11 @@ void PcapSource::OpenOffline()
 		return;
 		}
 
-	props.selectable_fd = fileno(pcap_file(pd));
-
-	if ( props.selectable_fd < 0 )
-		InternalError("OS does not support selectable pcap fd");
+	// We don't register the file descriptor if we're in offline mode,
+	// because libpcap's file descriptor for trace files isn't a reliable
+	// way to know whether we actually have data to read.
+	// See https://github.com/the-tcpdump-group/libpcap/issues/870
+	props.selectable_fd = -1;
 
 	props.link_type = pcap_datalink(pd);
 	props.is_live = false;