- User Since
- Aug 24 2017, 3:21 PM (86 w, 2 d)
Thu, Apr 11
I have a couple of thoughts I had while writing this.
Wed, Apr 10
Mar 19 2019
Mar 15 2019
Document the new CONST_STRING sysctl macros
Mar 14 2019
I like the addition of SYSCTL_ADD_CONST_STRING since it also ensures that we enforce that we don't have CTLFLAG_WR.
Use __DECONST instead of a poor reimplementation
This version actually compilers without the warnings, but it *is* rather ugly to have to do it this way
Introduce a __drop_const macro to handle the warning
Looks like the best 'fix' would be to use _Pragma to drop the warnings in SYSCTL_ADD_STRING. That will take a bit more work to get correct.
This still doesn't quite work 100%, because it causes a -Wcast-qual warning....
Also cast isc_driver_version in SYSCTL_ADD_STRING
Mar 13 2019
This needs some re-work before it's acceptable.
Uhh, oh I see why it's marked as non-const.... SYSCTL_ADD_STRING with a read-only flag doesn't know to take a const value....
Drivers can sort of get around this by using an explicit cast, but that triggers a -Wcast-qual warning.
Replace the 2048 magic number with MCLBYTES
add an MPASS so we trigger a panic when in debug
Mar 8 2019
Fix minor style nits
Mar 7 2019
The intention here is to ensure that device drivers which need the size of the rx buffers can ensure they use the same size as iflib, (possibly incase we want to re-enable something like the old CONTIG_MALLOCWORKS again)
Mar 5 2019
I'm definitely open to better/alternative solutions, and no, I don't actually have a live replication of the issue.
Move the remap++ to after the error check
I found this during code inspection while looking at a similar construction for a legacy device driver. It's possible this can never trigger, depending on how bus_dmamap_load_mbuf_sg returns EFBIG, but it definitely seemed plausible that it might happen, and potentially locking up a CPU is not a happy thing to do.
Feb 14 2019
I think it would be useful to actually expose the ifl_buf_size calculation somehow, since hardware needs this information. Currently, most drivers just calculate the value in the same way that iflib does, but that seems error prone if we ever do change this in the future.
Jan 28 2019
Dec 19 2018
We probably should only check CSUM_TSO for TCP packets..
I think I see what's wrong... we check CSUM_TSO for IPPROTO_UDP and IPPROTO_SCTP. That doesn't make sense, since TSO is only for TCP
We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.
Dec 17 2018
The following pkt-gen command:
Dec 14 2018
Stephen Hurd, this is what I believe to be the correct fix for this issue. I'm not entirely certain how to reproduce the original setup that triggered the bug, so some help with that would be appreciated.
Dec 6 2018
I know I'm late commenting on this, but I think the correct fix is to initialize tx_cidx_processed to txr->desc_count -1 when we initialize.
Nov 7 2018
Nov 5 2018
If I fix the driver to enable IP checksums on TSOs even when CSUM_IP isn't set, *and* I enable IFLIB_TSO_INIT_IP, then this patch isn't necessary.
Actually, I think the reason we set NEED_CSUM_ZERO in some hardware is only because they need the IP sum zero'd during TSO, not because they need it for non-TSO packets....
Nov 2 2018
Thanks! This looks great.
So this is a step in the right direction, but isn't a complete fix for the issue. We also need to enable the ip_sum workaround in this case as well. Possibly we can just move that check to after the TX_OFFLOAD4 and TX_OFFLOAD6 checks?
This doesn't seem like the right fix.
May 11 2018
- taskqgroup: make tqg_name a constant
- taskqgroup: mark name paramter of taskqgroup_attach as const
- iflib: mark irq allocation name parameter as constant
Looks like this involves a bit more cleanup to the taskqgroup code (since ultimately the name gets passed there). due to the scope I think I'll split it into two commits.
May 8 2018
Hmmm... I think there were. I'll double check this later today and see what I missed. I have a separate system for sending the reviews because my test system doesn't have direct access, I probably just forgot a few lines..
May 7 2018
May 4 2018
Remove the redundant message in iflib_device_register
Oops didn't mean to upload a new revision..
While we're cleaning this up, I'd either remove this print, or add a matching one for the tx setup, and remove the one in the caller. I realize that you can deduce which failed by how many prints you get, but that seems ... weird..
As mentioned on the other review, maybe remove the print in the caller, since it is now redundant..
Can we MFC this to stable/11? I originally found the bug on my FreeBSD 11.1 system.
May 3 2018
I've got a copy of the github mirror and wasnt sure how to convert the git commits back to SVN revisions, so I referenced the git commits where this was broken in my summary.