! MFp4 bz_ipv6_fast:
!
! Migrate udp6_send v4mapped code to udp6_output saving us a re-lock and
! further simplifying the af handling code by eliminating AF_INET checks
! at the tail end of the function.
!
! Rework output path locking similar to UDP4 allowing for better
! parallelism (see r222488).
!
! Cleanup early initializations, comments, ...
!
!
! Sponsored by: The FreeBSD Foundation (2012)
! Sponsored by: iXsystems (2012)
!
! Reviewed by:
! MFC After: 6 weeks
Details
Details
I did a couple of brief test runs on NFS root rabbit4 with a GENERIC kernel (with all debugging turned on) using pps into a discard device. Went from 410kpps to 700+kpps.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Poke. If this stays silent I'll commit it after at the end of the year and I'll still be happy about post-commit review.
Comment Actions
Seems generally sensible. Better documenting the locking protocol would be useful -- why various locks are required, and what the implications of the unlock flag are (they are more broad than suggested by the name). Addition of a flags argument that is conflated with a local flags field is confusing.
| udp6_usrreq.c | ||
|---|---|---|
| 626 ↗ | (On Diff #8906) | There's a slightly more complex locking protocol going here than just the UDP hash lock (UH_), and it would be worth a comment to explain it. I.e., the implied state of the inpcb lock for each. |
| 652 ↗ | (On Diff #8906) | Perhaps "In contrast to IPv4, we..."? |
| 731 ↗ | (On Diff #8906) | Note that there is a small but real race here, similar to the one commented above, where we hold no lock on 'inp' and hence should really recheck the if() that led us down this path. This race existed before your refactoring as well. A similar comment is probably worth dropping in here. |
| 738 ↗ | (On Diff #8906) | In UDPv4, a local port of 0 would force a connection to be set up, so a write lock would in fact still be required in this case. It sounds like, from the comment below ("Since we saw..."), a clarifying comment on why each of the above cases is the way it is wouldn't hurt. |
| 779 ↗ | (On Diff #8906) | If it can happen, then, indeed, this code is wrong. It wouldn't hurt, if you're going to leave the if() here in place, to have the required locking assertion in it, so that we get the desired resulting panic? |
| 929 ↗ | (On Diff #8906) | Use of 'flags' in this function probably deserves a comment -- or, perhaps, two variables, as the 'flags' argument introduced above is not really the same 'flags' used here or passed to the transmit routine further down. I'd be tempted to rename the initial 'flags' to 'flags_arg' or such? |
| 945 ↗ | (On Diff #8906) | It would be nice to catch the UH_UNLOCKED invariant better here; e.g., a final assert of some sort? |