diff --git a/CHANGES b/CHANGES index c6a74518de..f9466f7253 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index b49614a894..2956c541ca 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.0-dev.185 +6.0.0-dev.189 diff --git a/src/iosource/PktSrc.cc b/src/iosource/PktSrc.cc index c7c42edd68..a7940250df 100644 --- a/src/iosource/PktSrc.cc +++ b/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"); - } + 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(¤t_packet) ) { + had_packet = true; + if ( current_packet.time < 0 ) { Weird("negative_packet_timestamp", ¤t_packet); @@ -191,6 +189,10 @@ bool PktSrc::ExtractNextPacketInternal() have_packet = true; return true; } + else + { + had_packet = false; + } if ( run_state::pseudo_realtime && ! IsOpen() ) { @@ -276,39 +278,41 @@ 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 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 ) + { + ExtractNextPacketInternal(); + + // This duplicates the calculation used in run_state::check_pseudo_time(). + double pseudo_time = current_packet.time - run_state::detail::first_timestamp; + double ct = (util::current_time(true) - run_state::detail::first_wallclock) * + run_state::pseudo_realtime; + 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 ( ! pkt_available && ! run_state::pseudo_realtime ) - return 0.00002; + if ( have_packet || had_packet ) + return 0.0; + + 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; - - // This duplicates the calculation used in run_state::check_pseudo_time(). - double pseudo_time = current_packet.time - run_state::detail::first_timestamp; - double ct = (util::current_time(true) - run_state::detail::first_wallclock) * - run_state::pseudo_realtime; - return std::max(0.0, pseudo_time - ct); + // 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 diff --git a/src/iosource/PktSrc.h b/src/iosource/PktSrc.h index 0cacba7845..d93b5d60bc 100644 --- a/src/iosource/PktSrc.h +++ b/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 filters; diff --git a/src/iosource/pcap/Source.cc b/src/iosource/pcap/Source.cc index 4ebd8e77ca..fd01c19554 100644 --- a/src/iosource/pcap/Source.cc +++ b/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;