Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/6790 add worker sync on finish v3 #10628

Closed

Conversation

lukashino
Copy link
Contributor

Follow-up of: #10564
Ticket: #6790
https://redmine.openinfosecfoundation.org/issues/6790

Describe changes:
v3

  • refactored the receive loop

v2

  • added a pointer check after calloc operation
  • improved the reasoning in the commit message

v1

  • worker synchronization added before the port is closed
  • in the peered modes, the thread only closes the peered port - it cannot close its own port because it might still be used by the other peered threads

Lukas Sismis added 2 commits March 12, 2024 23:25
When Suricata was running in IPS mode and received a signal to stop,
the first worker of every interface/port stopped the port and
proactively stopped the peered interface as well.
This was done to be as accurate with port stats as possible.
However, in a highly active scenarios (lots of packets moving around)
the peered workers might still be in the process of a packet
release operation. These workers would then attempt to transmit
on a stopped interface - resulting in an errorneous operation.

Instead, this patch proposes a worker synchronization of the given
port. After these workers are synchronized, it is known that no packets
will be sent of the peered interface, therefore the first worker can
stop it. This however cannot be assumed about "its own" port as the
peered workers can still try to send the packets. Therefore, ports
are only stopped by the peered workers.

Ticket: OISF#6790
@lukashino lukashino requested a review from victorjulien as a code owner March 13, 2024 08:43
static void DPDKSegmentedMbufWarning(struct rte_mbuf *mbuf)
{
static bool segmented_mbufs_warned = false;
if (!rte_pktmbuf_is_contiguous(mbuf) && !segmented_mbufs_warned) {
Copy link
Member

Choose a reason for hiding this comment

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

how expensive is rte_pktmbuf_is_contiguous? May be cheaper to check segmented_mbufs_warned first?

pwd_zero_rx_packet_polls_count++;
if (pwd_zero_rx_packet_polls_count <= MIN_ZERO_POLL_COUNT)
continue;
static void DPDKSegmentedMbufWarning(struct rte_mbuf *mbuf)
Copy link
Member

Choose a reason for hiding this comment

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

inline? Runs for each pkt I think

* \brief Initializes a packet from an mbuf
* \return true if the packet was initialized successfully, false otherwise
*/
static bool PacketInitFromMbuf(DPDKThreadVars *ptv, struct rte_mbuf *mbuf, Packet **pkt_out)
Copy link
Member

Choose a reason for hiding this comment

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

inline?

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few comments inline

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 19303

@lukashino
Copy link
Contributor Author

I inlined your comments, let's continue in #10642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants