Intel1g supporting multiple processes and RSS #897
Conversation
|
Big thumbs up for RSS :) |
|
From a driver users perspective does it look like the kind of interface you would want, particularly if it was replicated into the 82599 driver? |
|
Cool! Pioneering a new design for multiprocess drivers :-) Personally I find the plumbing rather stylish with using cc @hb9cwp who is surely affected by this driver rewrite and likely has feedback too? |
petebristow
changed the title
|
This driver is now in reasonably good shape. An assignee and review would be great. |
|
@petebristow The top-level comment in intel1g.lua says this supports both Intel I350 and 82599 (awesome?), but it does not implement VMDq as does does the existing Intel10G driver, am I right? If my understanding is correct, maybe rename the module to A section about this driver in |
| } __attribute__((packed)) | ||
| ]]) | ||
| assert(ffi.sizeof(rxdesc_t), | ||
| "sizeof(rxdesc_t)= ".. ffi.sizeof(rxdesc_t) .. ", but must be 16 Byte") |
There was a problem hiding this comment.
Should be assert(ffi.sizeof(rxdesc_t) == 16, ...)? Not sure how much sense the assertion makes though, in which cases would it not hold true?
|
This is the first driver that can support a herd of snabb processes. Do you envisage a need for a herd of VMDq processes as well? I've started to manage the herd with something a little like
@lukego Is the Mellanox driver going to take a similar approach to multiple processes? |
|
@petebristow I am fine with |
|
@petebristow If you merge #942 SnabbBot will run selftest.sh. |
| assert((self.ndesc %128) ==0, | ||
| "ndesc must be a multiple of 128 (for Rx only)") -- see 7.1.4.5 | ||
|
|
||
| self.rxpackets = ffi.new("struct packet *[?]", self.ndesc) |
There was a problem hiding this comment.
Hm. What do you see as the main advantage of doing it that way?
There was a problem hiding this comment.
Seems cleaner to me to not invoke the C name directly, in case the ever definition changes. Also, if desq_t was defined module-wide, then the JIT would see one ctype instead of different ctypes for each app instance.
There was a problem hiding this comment.
Ok; the latter is a good enough reason for me. :-)
There was a problem hiding this comment.
Never mind my advice above, as we have decided against exposing packet_t, see #996 (comment).
…ometimes it runs slow in absolute terms which needs timeline for further investigation, hence the 400000pps test. The test was written on a very fast consumer CPU rather than a slower per core server CPU, 200000 should pass on E5 chips ok
The timer() function now sets its new deadline based on the current time rather than the previous deadline. This is more consistent with the documentation and prevents timeouts from being "saved up" in the style of a token bucket when the timer is not called for a period of time.
These are convenience functions for the most common timer use cases. timeout(seconds) returns true after the time has elapsed. throttle(seconds) returns true at regular intervals.
Fix a problem where engine.now() could return a stale value if the engine is not running i.e. timestamp of the last breath. Now the engine only uses its cached time value while the breathe loop is active and otherwise it fetches the current time from the time source. (Could also consider always returning the latest time without caching but this would potentially make engine.now() too expensive...?)
This function is unused due to the previous commits.
…tting counter registers
…xported, mac address is export via a counter
Lots to be done, thoughts would be great, the tests are currently the best guide to usage, particularly
test6.sh. It's fairly opinionated.selftest()andselftest.shcould do with being extended. The containedselftest.shseems adequate for this.