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

Rework of PfRingDevice capture thread implementation. #1668

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

Fixes #1664.

  • Added StopToken and StopTokenSource that mirror C++20's stop_token and stop_source.
  • Reworked the capture thread function into a free function to simplify interactions.
    Many variables are now kept on the thread's stack instead of relying on member fields in PfRindDevice.
  • Extracted the code for setting core affinity into a function.
  • Cleaned up capture thread startup.

- Completely rewrote the capture thread function to be a free function.
- Added StopToken and StopTokenSource to encapsulate stop requests.
- Refactored startCaptureSingleThread to use startCaptureMultiThread.
@Dimi1010 Dimi1010 changed the title Rework of PF_RING capture thread implementation. Rework of PfRingDevice capture thread implementation. Dec 21, 2024
Copy link

codecov bot commented Dec 21, 2024

Codecov Report

Attention: Patch coverage is 98.38710% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.17%. Comparing base (ab75534) to head (baeb36c).

Files with missing lines Patch % Lines
Pcap++/header/StopToken.h 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1668      +/-   ##
==========================================
+ Coverage   83.15%   83.17%   +0.01%     
==========================================
  Files         277      280       +3     
  Lines       48193    48255      +62     
  Branches     9949     9969      +20     
==========================================
+ Hits        40077    40137      +60     
+ Misses       7225     7222       -3     
- Partials      891      896       +5     
Flag Coverage Δ
alpine320 75.16% <94.44%> (+0.01%) ⬆️
fedora40 75.18% <94.73%> (-0.02%) ⬇️
macos-13 80.68% <100.00%> (+0.02%) ⬆️
macos-14 80.68% <100.00%> (+0.02%) ⬆️
macos-15 80.65% <100.00%> (+0.02%) ⬆️
mingw32 70.90% <93.75%> (-0.02%) ⬇️
mingw64 70.86% <93.75%> (-0.02%) ⬇️
npcap 85.32% <100.00%> (+<0.01%) ⬆️
rhel94 75.06% <94.73%> (+0.01%) ⬆️
ubuntu2004 58.63% <93.75%> (+0.01%) ⬆️
ubuntu2004-zstd 58.75% <93.75%> (+0.02%) ⬆️
ubuntu2204 74.99% <94.73%> (+0.02%) ⬆️
ubuntu2204-icpx 61.45% <100.00%> (+0.05%) ⬆️
ubuntu2404 75.21% <94.73%> (-0.01%) ⬇️
unittest 83.17% <98.38%> (+0.01%) ⬆️
windows-2019 85.35% <100.00%> (+<0.01%) ⬆️
windows-2022 85.38% <100.00%> (+<0.01%) ⬆️
winpcap 85.35% <100.00%> (+0.01%) ⬆️
xdp 50.48% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dimi1010 Dimi1010 marked this pull request as ready for review December 24, 2024 14:37
@Dimi1010
Copy link
Collaborator Author

I don't actually have a system that can run PF_RING. So it would be nice if someone could test it live.

Copy link
Owner

@seladb seladb left a comment

Choose a reason for hiding this comment

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

@Dimi1010 this PR does a lot of things and could be broken up into multiple PRs to make it easier to review.

In order to solve #1664, wouldn't it be enough to create a shared_ptr from mutex, cond and startThread and pass them to the worker threads? 🤔

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Jan 1, 2025

@Dimi1010 this PR does a lot of things and could be broken up into multiple PRs to make it easier to review.

In order to solve #1664, wouldn't it be enough to create a shared_ptr from mutex, cond and startThread and pass them to the worker threads? 🤔

I suppose it can be split up.

The main fix is the startup block struct. The rest like the StopToken was added as the whole startThread being 0, 1 or 2 was a bit hard to read.

@seladb
Copy link
Owner

seladb commented Jan 1, 2025

@Dimi1010 this PR does a lot of things and could be broken up into multiple PRs to make it easier to review.
In order to solve #1664, wouldn't it be enough to create a shared_ptr from mutex, cond and startThread and pass them to the worker threads? 🤔

I suppose it can be split up.

The main fix is the startup block struct. The rest like the StopToken was added as the whole startThread being 0, 1 or 2 was a bit hard to read.

I wouldn't add this much code just to make startThread more readable... if we can first fix #1664, potentially by using shared_ptr, it could make a great first PR. Then we can open additional PRs to add the rest of the fixes. Please let me know what you think

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

Successfully merging this pull request may close these issues.

PfRingDevice::startCaptureSingleThread maybe crash after exits the funtion
2 participants