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

ON-16111: use monitor thread in zfsink to allow testing higher rates #50

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

rdrinkwa-xilinx
Copy link

Modified zfsink to use a monitoring thread to output traffic rate every second, as with efsink in Onload.

Tested with high packet rate (>5Mpps) and high throughput (>9Gbps) to confirm output is correctly formatted.
Timestamps work as before and are output every packet.
Monitoring thread can be disabled using '-q'.

@rdrinkwa-xilinx rdrinkwa-xilinx requested a review from a team as a code owner October 15, 2024 15:12
Copy link
Contributor

@pemberso-xilinx pemberso-xilinx left a comment

Choose a reason for hiding this comment

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

The only data used by the monitor thread is n_rx_pkts and n_rx_bytes but you've created a data structure with a lot more in it.

I think this update would be possible with more minimal changes to the app if just n_rx_pkts and n_rx_bytes were shared.

Could you also add some examples of the output from testing?

@rdrinkwa-xilinx
Copy link
Author

I refactored the code to just pass the resource structure between functions because this felt cleaner to me and was also the approach used for efsink.

Result from testing different options:

[server1:static]$ ./zfsink 224.1.2.3:12345 # two bursts of 100 packets
# pkt-rate  bandwidth(Mbps)       total-pkts
         0                0                 0
         0                0                 0
       100                0               100
         0                0               100
       100                0               200
         0                0               200
^C
[server1:static]$ ./zfsink -m 224.1.2.3:12345 # two bursts of 100 packets
# pkt-rate  bandwidth(Mbps)       total-pkts
         0                0                 0
         0                0                 0
       100                0               100
       100                0               200
         0                0               200
^C
[server1:static]$ ./zfsink -w 224.1.2.3:12345 # two bursts of 100 packets
# pkt-rate  bandwidth(Mbps)       total-pkts
         0                0                 0
         0                0                 0
       100                0               100
       100                0               200
         0                0               200
         0                0               200
^C
[server1:static]$ ./zfsink -r 224.1.2.3:12345 # one burst of 5 packets
# pkt-rate  bandwidth(Mbps)       total-pkts
         0                0                 0
         0                0                 0
Hardware timestamp: 1729093248.149723774
Hardware timestamp: 1729093248.149724426
Hardware timestamp: 1729093248.149724499
Hardware timestamp: 1729093248.149724575
Hardware timestamp: 1729093248.149724651
         5                0                 5
         0                0                 5
         0                0                 5
^C
[server1:static]$ ./zfsink -qr 224.1.2.3:12345 # one burst of 5 packets
Hardware timestamp: 1729093258.533324537
Hardware timestamp: 1729093258.533325183
Hardware timestamp: 1729093258.533325258
Hardware timestamp: 1729093258.533325333
Hardware timestamp: 1729093258.533325410
^C
[server1:static]$ ./zfsink 224.1.2.3:12345 # efsend using 1 byte payload no throttling
# pkt-rate  bandwidth(Mbps)      total-pkts
         0                0                0
         0                0                0
         0                0                0
         0                0                0
   3534625               28          3534625
   5248144               41          8782769
   5247216               41         14029985
   5248816               41         19278801
   5248128               41         24526929
^C
[server1:static]$

@pemberso-xilinx
Copy link
Contributor

Thanks for the output, looks fine.

I refactored the code to just pass the resource structure between functions because this felt cleaner to me and was also the approach used for efsink.

efsink needs to manage packet buffers so there is more need to share state. I'd rather have a minimal code change here - e.g. suggest just adding a couple of global stats counters and then most of the changes to existing code won't be needed. Code can always be refactored later if more data needs to be shared but doesn't seem necessary here.

src/tests/zf_apps/zfsink.c Outdated Show resolved Hide resolved
src/tests/zf_apps/zfsink.c Outdated Show resolved Hide resolved
@@ -290,6 +347,8 @@ int main(int argc, char* argv[])
if( cfg_rx_timestamping )
ZF_TRY(zf_attr_set_int(attr, "rx_timestamping", 1));

ZF_TRY((res = calloc(1, sizeof(*res))) != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is just a small test app, I think we could just have a single global instance of struct resources rather than dynamically allocating and passing around the functions.

Copy link
Contributor

@ligallag-amd ligallag-amd 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 to me



static bool cfg_quiet = false;
static bool cfg_rx_timestamping = false;
static struct resources res;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor comment - I would use

static struct resources res = {0};

here and save the lines initialising later in main()

Copy link
Contributor

Choose a reason for hiding this comment

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

Objects with static storage duration will implicitly be empty-initialised, so the current declaration is sufficient to set the values to 0.

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.

4 participants