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

nvnetdrv v2 part 1 #686

Merged
merged 4 commits into from
Dec 29, 2024
Merged

nvnetdrv v2 part 1 #686

merged 4 commits into from
Dec 29, 2024

Conversation

Ryzee119
Copy link
Contributor

@Ryzee119 Ryzee119 commented Dec 24, 2024

This is a culmination of work over ~12 months or so.

With nvnetdrv being used in StellarOS, it has seen many hundreds/thousands of people using it through the online updater and FTP server and a few edges cases started arising.

As part of the debugging effort in collaboration with LoveMhz/StellarOS developers, a significant amount of refactoring and optimizations were done.

This has turned into a reasonably big PR (sorry!) so I'm calling it version 2, however the primary changes are

  1. Refactoring of variable names
  2. Reduced memory footprint significantly
  3. Less threads/context switching
  4. Fixes some bugs.

Hopefully I have broken up the commits into logical units.

My stress test involved 8 x parallel iperf connections, 10 parallel FTP connections (with hash check of files after), web server and sntp client at the same time.

Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

I've only managed to go through up to and including nvnetdrv: Soft error if framing error only and left some feedback.

When that is merge-able I'll probably split that off into a separate PR to make this a bit easier to review.

lib/net/nvnetdrv/nvnetdrv.c Outdated Show resolved Hide resolved
lib/net/nvnetdrv/nvnetdrv.c Outdated Show resolved Hide resolved
@Ryzee119 Ryzee119 changed the title nvnetdrv v2 nvnetdrv v2 part 1 Dec 27, 2024
@Ryzee119
Copy link
Contributor Author

Ryzee119 commented Dec 27, 2024

When that is merge-able I'll probably split that off into a separate PR to make this a bit easier to review.

Ive renamed this to part one and dropped the other commits. Will raised another when merged and rebase the other commits

Comment on lines 83 to 84
static struct descriptor_t *g_rxRing;
static struct descriptor_t *g_txRing;
Copy link
Member

@thrimbor thrimbor Dec 29, 2024

Choose a reason for hiding this comment

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

Hm, I don't think we can drop the volatile here as the hw modifies the flags member. Interestingly this change tests fine even with LTO, probably because nvnetdrv_handle_rx_irq copies the address into a proper volatile pointer.

Imho the initialization of these should be changed, too, as it's currently casting to a non-volatile pointer, if you want to squeeze that into one of the commits in this PR:

// Setup the TX and RX ring descriptor pointers
    g_rxRing = (struct descriptor_t *)descriptors;
    g_txRing = (struct descriptor_t *)descriptors + RX_RING_SIZE;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yep gotcha, fixed thanks

@thrimbor
Copy link
Member

Looks good to me, merging

@thrimbor thrimbor merged commit 19195a0 into XboxDev:master Dec 29, 2024
6 checks passed
@Ryzee119 Ryzee119 mentioned this pull request Dec 29, 2024
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.

2 participants