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

Intel10G driver: multi-iovec and jumbo frames #161

Merged
merged 1 commit into from
May 4, 2014

Conversation

javierguerragiraldez
Copy link
Contributor

This handles some issues on "bigger than trivial" packets.

  • sets the maximum frame size (register MAXFRS) to 9000+18 bytes. It could be nice to use a configurable option, but it's a chip-wide setting, so it's likely better to simply set to the max.
  • fixes the too-many-releasing bug by incrementing the reference counter of the packet for each descriptor in the transmit queue. This makes it possible to send a multi-iovec packet without coalescing it first.
  • allows the NIC to split packets that are bigger than a single buffer (currently 4K). Both :can_receive() and :receive() have tight loops to check for an EOP descriptor after some ready-but-partial descriptors. This could be manually unrolled (just 3 iterations with current defaults) if benchmarks prove it to be a performance regression.
  • implements the :stop() method for the app, in the VF case, it calls the PF if it's the last VF. This is necessary to allow changing configurations. (seem unrelated, but some settings (frame size in this case) persist even across process invocations, so it's impossible to properly test it without :stop())

@lukego
Copy link
Member

lukego commented May 4, 2014

Great!

Requests:

Could you add more detail to the pull request so that it can serve as release notes? e.g. title "Intel10G: Added support for multi-iovec packets and jumbo frames" and text saying if any configuration is needed etc?

Reading the code, the main change I see is adding support for stop()ing VFs. That is not mentioned in the pull request. Can you split that out?

@javierguerragiraldez
Copy link
Contributor Author

is it better now?

@lukego
Copy link
Member

lukego commented May 4, 2014

Looks great!

How about the Travis-CI failing test?

@lukego
Copy link
Member

lukego commented May 4, 2014

It would also be nice to have the detailed description as the commit message so that we see it in "Git log". It seems like a bother to have to duplicate the text for both Commit and Pull Request. I wonder how people usually deal with this.

This handles some issues on "bigger than trivial" packets.

- sets the maximum frame size (register MAXFRS) to 9000+18 bytes.  It could be nice to use a configurable option, but it's a chip-wide setting, so it's likely better to simply set to the max.

- fixes the too-many-releasing bug by incrementing the reference counter of the packet for each descriptor in the transmit queue.  This makes it possible to send a multi-iovec packet without coalescing it first.

- allows the NIC to split packets that are bigger than a single buffer (currently 4K).  Both `:can_receive()` and `:receive()` have tight loops to check for an EOP descriptor after some ready-but-partial descriptors.  This could be manually unrolled (just 3 iterations with current defaults) if benchmarks prove it to be a performance regression.

- implements the `:stop()` method for the app, in the VF case, it calls the PF if it's the last VF.  This is necessary to allow changing configurations.  (seem unrelated, but some settings (frame size in this case) persist even across process invocations, so it's impossible to properly test it without `:stop()`)
@lukego
Copy link
Member

lukego commented May 4, 2014

One more request: Could you update the title to say that these changes relate to the Intel 10G driver?

@javierguerragiraldez javierguerragiraldez changed the title multi-iovec and jumbo frames Intel10G driver: multi-iovec and jumbo frames May 4, 2014
@javierguerragiraldez
Copy link
Contributor Author

just extended the commit message. also fixed the failing Travis CI (it was failing at sending the 'skip' result, duh!), but now it failed on the packet filter app. :-(

lukego added a commit that referenced this pull request May 4, 2014
Intel10G driver: multi-iovec and jumbo frames
@lukego lukego merged commit c327928 into snabbco:master May 4, 2014
@javierguerragiraldez javierguerragiraldez deleted the multi_iovec branch June 3, 2014 08:22
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.

None yet

2 participants