pkelsey (Patrick Kelsey)
User

Projects

User Details

User Since
Jun 2 2014, 3:47 PM (189 w, 1 d)

Recent Activity

Thu, Jan 4

pkelsey added inline comments to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.
Thu, Jan 4, 3:57 AM

Tue, Jan 2

pkelsey added inline comments to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.
Tue, Jan 2, 9:44 PM
pkelsey added reviewers for D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation: hiren, tuexen.
Tue, Jan 2, 9:25 PM
pkelsey added a comment to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.

Awesome! Thank you for this work!

Now, sorry to be a PITA but you should separate out this into 3 reviews: 1) fast-open ON by default removing TCP_RFC7413 2) Bug fix for the issue in tcp_fastopen_check_cookie() 3) Client side TFO implementation.

I can do this I suppose, although I have to say it's hard for me to see the value in doing it as (2) and (3) seem pretty easy to evaluate in the current patchset, as (2) consists of moving a lock acquisition earlier in a small function and ensuring release on all the exit paths, and (3) is the mechanical removal of #ifdefs and the related rename of a kernel option that is used in one place. Perhaps because I don't think I fully understand the practical motivation for this separation, I'm also not sure whether or not it matters how I synthesize the new versions of the code - should they all be independent diffs against -current (which seems weird as it can hide interactions between the changes from review), or is there some preferred sequence (should the locking bug be presented as happening before or after all the other changes to same function)?

Motivation: easier on reviewers.
Also, I don't think you'd want to commit this as is, do you? These are 3 separate changes and I prefer 3 different commits. But as you are the one doing actual work, you can decide how to go about it. :-)
I'd get 2) and 3) committed first as they are small and then generate 1) for larger review. But again, your call. :-)

Tue, Jan 2, 7:40 PM
pkelsey added a comment to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.

I agree with hiren... Splitting this up in three patch sets makes a lot of sense and makes the review much simpler. While doing that, you could also change the name of the sysctl variables net.inet.tcp.fastopen.server_enabled to net.inet.tcp.fastopen.server_enable and net.inet.tcp.fastopen.client_enabled to net.inet.tcp.fastopen.client_enable to be consistent with other sysctl variables controlling the usage of features.

With respect to 'enable' vs 'enabled', I considered this issue, but I didn't find the consistency you are claiming. I saw that both are used, and I was unable to discern any rule as to when one was employed versus the other, so I picked the one that reads as a status indicator as that seemed like it would be the more frequent reason they would be read. As seen on an r327445 kernel (with my TFO patches applied, hence the grep -v), 'enabled' is used in 8 names (5 of them are loader tunables, and an overlapping 5 are runtime writeable):

$ sysctl -aN | grep -v fastopen | grep enabled | wc -l

8
Tue, Jan 2, 6:18 PM
pkelsey added a comment to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.

I agree with hiren... Splitting this up in three patch sets makes a lot of sense and makes the review much simpler. While doing that, you could also change the name of the sysctl variables net.inet.tcp.fastopen.server_enabled to net.inet.tcp.fastopen.server_enable and net.inet.tcp.fastopen.client_enabled to net.inet.tcp.fastopen.client_enable to be consistent with other sysctl variables controlling the usage of features.

Tue, Jan 2, 5:59 PM
pkelsey added a comment to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.

Awesome! Thank you for this work!

Now, sorry to be a PITA but you should separate out this into 3 reviews: 1) fast-open ON by default removing TCP_RFC7413 2) Bug fix for the issue in tcp_fastopen_check_cookie() 3) Client side TFO implementation.

Tue, Jan 2, 5:47 PM

Sun, Dec 31

pkelsey created D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.
Sun, Dec 31, 8:56 PM

Oct 1 2017

pkelsey committed rS324181: The soisconnected() call removed from syncache_socket() in r307966 was.
The soisconnected() call removed from syncache_socket() in r307966 was
Oct 1 2017, 11:37 PM

Jun 5 2017

pkelsey committed rD50322: Update my PGP key..
Update my PGP key.
Jun 5 2017, 5:08 PM

Apr 25 2017

pkelsey closed D10322: Remove unnecessary check for NULL mbuf in soreceive_generic() by committing rS317421: Remove unnecessary check for NULL mbuf in soreceive_generic()..
Apr 25 2017, 7:54 PM
pkelsey committed rS317421: Remove unnecessary check for NULL mbuf in soreceive_generic()..
Remove unnecessary check for NULL mbuf in soreceive_generic().
Apr 25 2017, 7:54 PM

Apr 16 2017

pkelsey committed rS317035: Fix userland tools that don't check the format of routing socket.
Fix userland tools that don't check the format of routing socket
Apr 16 2017, 7:17 PM
pkelsey closed D10330: Fix bugs in routing socket use by userland tools by committing rS317035: Fix userland tools that don't check the format of routing socket.
Apr 16 2017, 7:17 PM

Apr 10 2017

pkelsey committed rD50150: Document __FreeBSD_version 1200028.
Document __FreeBSD_version 1200028
Apr 10 2017, 6:18 PM
pkelsey committed rS316683: Bump __FreeBSD_version due to r316648, rename of.
Bump __FreeBSD_version due to r316648, rename of
Apr 10 2017, 5:59 PM

Apr 9 2017

pkelsey updated the diff for D10330: Fix bugs in routing socket use by userland tools.

break -> continue in the addition to sbin/routed/table.c

Apr 9 2017, 6:09 PM
pkelsey added inline comments to D10330: Fix bugs in routing socket use by userland tools.
Apr 9 2017, 6:02 PM
pkelsey added a comment to D10330: Fix bugs in routing socket use by userland tools.
In D10330#213998, @ae wrote:

Looks good to me. Also I want to note, that all

do 
 read();
} while()

loops are affected to the problem described in rS303374. It would be nice to fix this problem too. :)

Apr 9 2017, 5:48 PM
pkelsey added inline comments to D10330: Fix bugs in routing socket use by userland tools.
Apr 9 2017, 5:01 PM
pkelsey updated the summary of D10330: Fix bugs in routing socket use by userland tools.
Apr 9 2017, 6:51 AM
pkelsey added a reviewer for D10330: Fix bugs in routing socket use by userland tools: network.
Apr 9 2017, 6:48 AM
pkelsey added a member for network: pkelsey.
Apr 9 2017, 6:48 AM
pkelsey created D10330: Fix bugs in routing socket use by userland tools.
Apr 9 2017, 6:46 AM
pkelsey committed rS316648: Corrected misspelled versions of rendezvous..
Corrected misspelled versions of rendezvous.
Apr 9 2017, 2:00 AM
pkelsey closed D10313: rendevous -> rendezvous by committing rS316648: Corrected misspelled versions of rendezvous..
Apr 9 2017, 2:00 AM

Apr 8 2017

pkelsey added a reviewer for D10322: Remove unnecessary check for NULL mbuf in soreceive_generic(): bms.
Apr 8 2017, 6:02 PM
pkelsey created D10322: Remove unnecessary check for NULL mbuf in soreceive_generic().
Apr 8 2017, 6:00 PM
pkelsey created D10318: Remove redundant checks of rtm_type in route_output().
Apr 8 2017, 3:24 PM
pkelsey committed rS316634: Fixed typo in comment found while reading commit email for fix of.
Fixed typo in comment found while reading commit email for fix of
Apr 8 2017, 4:51 AM
pkelsey committed rS316633: Fixed typo in comment..
Fixed typo in comment.
Apr 8 2017, 4:46 AM
pkelsey committed rS316632: Fixed typo..
Fixed typo.
Apr 8 2017, 4:42 AM
pkelsey committed rS316631: Fix typo in comment..
Fix typo in comment.
Apr 8 2017, 4:37 AM
pkelsey committed rS316630: Fix typo..
Fix typo.
Apr 8 2017, 4:34 AM
pkelsey created D10313: rendevous -> rendezvous.
Apr 8 2017, 4:10 AM

Feb 15 2017

pkelsey accepted D9606: Remove compatibility with old libpcap..

Previously the code is hosted at Google code and that become read-only.

I think Luigi have another repository here: https://github.com/luigirizzo/netmap-libpcap but the code haven't been updated for some time.

The main motivation is that the pcap internal interfaces changes quite frequently, and it have become more and more burdensome to maintain the compatibility shims with older pcap versions (see rS313695, for instance, that we technically need additional #ifdef's to maintain compatibility with older pcap versions).

Feb 15 2017, 8:10 PM
pkelsey added a comment to D9606: Remove compatibility with old libpcap..

This makes sense to me from the standpoint of the removed code being unnecessary given that it is being maintained as part of a version of libpcap that doesn't require it.

Feb 15 2017, 2:40 PM
pkelsey accepted D9465: Remove unnecessary ifdef soup from struct tcpcb.
Feb 15 2017, 1:29 AM
pkelsey added a comment to D9465: Remove unnecessary ifdef soup from struct tcpcb.

We are trying to keep the same size and layout for a given machine type. The issue is that the TCPPCAP stuff used up uint64_t padding, and consumed different amounts of it on LP64 and LP32 systems, hence the #ifdef.

Feb 15 2017, 1:27 AM

Feb 14 2017

pkelsey added a comment to D9465: Remove unnecessary ifdef soup from struct tcpcb.

I don't have any TFO-specific complaints with removing those #ifdefs. I put them there out of uncertainty about any existing policy on userland-ABI concerns and they made it through code review. In hindsight, the #else parts of the #ifdefs should have had two items - t_Xopaque[] and t_Xspare[], because I was never of the mind that the space used by TFO might ever be used instead by something else that would then could only be enabled mutually-exclusively with TFO.

Feb 14 2017, 10:06 PM

Feb 9 2017

pkelsey added a comment to D5017: More than 65K connection from single application.

I think that "collision" is ok.

Of course it IS a new behaviour, but that is what we are trying to get. new behaviour.

Feb 9 2017, 5:55 PM · network
pkelsey added a comment to D5017: More than 65K connection from single application.

This is the source comment that should have been submitted with my 'request for change' action, had I pressed the buttons in a way that didn't dispose of it instead.

Feb 9 2017, 5:23 PM · network
pkelsey requested changes to D5017: More than 65K connection from single application.
Feb 9 2017, 5:15 PM · network

Feb 3 2017

pkelsey committed rS313168: Fix VIMAGE-related bugs in TFO. The autokey callout vnet context was.
Fix VIMAGE-related bugs in TFO. The autokey callout vnet context was
Feb 3 2017, 5:03 PM

Oct 15 2016

pkelsey committed rS307337: Fix cases where the TFO pending counter would leak references, and eventually….
Fix cases where the TFO pending counter would leak references, and eventually…
Oct 15 2016, 1:41 AM
pkelsey closed D8235: Fix TFO pending counter leaks by committing rS307337: Fix cases where the TFO pending counter would leak references, and eventually….
Oct 15 2016, 1:41 AM

Oct 14 2016

pkelsey added a member for transport: pkelsey.
Oct 14 2016, 2:04 AM

Oct 13 2016

pkelsey updated the diff for D8235: Fix TFO pending counter leaks.

Adjusted label names and comments

Oct 13 2016, 7:23 PM

Oct 12 2016

pkelsey retitled D8235: Fix TFO pending counter leaks from to Fix TFO pending counter leaks.
Oct 12 2016, 10:56 PM
pkelsey added a comment to D8218: Close t_tfo_pending leaks.
In D8218#170900, @jtl wrote:

This is functionally equivalent, but I don't think it is clearer. I think it is less clear that the conditional decrement is placed after the tfo_done label as it will never execute after a goto tfo_done.

I placed it there as a matter of future-proofing. It doesn't hurt anything for it to be after the tfo_done label. But, if someone makes a code change to add a new goto tfo_done, it will still hit this check.

Because you obviously have strong feelings for exactly how this code should be (and this doesn't really impact me), I've opened a bug and assigned it to you. You can fix it at your convenience.

Oct 12 2016, 8:52 PM
pkelsey added a comment to D8218: Close t_tfo_pending leaks.

This is functionally equivalent, but I don't think it is clearer. I think it is less clear that the conditional decrement is placed after the tfo_done label as it will never execute after a goto tfo_done. There is clear one-to-one correspondence between creating a TFO socket and not needing this decrement that I think is blurred by this positioning because the decrement is not an action that is common to 'done' and 'tfo_done', it is unique to 'done'. I think we would be better off if the code wasn't arranging to cancel a conditional action because it was placed in a path it never needs to be in.

Oct 12 2016, 7:05 PM

Oct 11 2016

pkelsey added a comment to D8218: Close t_tfo_pending leaks.
In D8218#170623, @jtl wrote:

I can only identify one circumstance where the t_tfo_pending counter on a listen socket will be incremented without a corresponding decrement ever occurring - when all of the following conditions are met:

  1. TFO is enabled in the system
  2. A TFO SYN with an invalid TFO cookie matches a listen socket that has TFO enabled
  3. The current t_tfo_pending count on that socket is <= so_qlimit / 2

The other way is...

  1. TFO enabled
  2. t_tfo_pending count <= so_qlimit / 2
  3. SYN without a TFO option, followed by a SYN with a valid TFO cookie and the same IPs and port numbers

See line-specific comments in the patch.

I don't see in-line comments. Perhaps you can resubmit them?

Oct 11 2016, 7:59 PM
pkelsey added a comment to D8218: Close t_tfo_pending leaks.

There is still a path that leaks when mac_syncache_init() fails.

Oct 11 2016, 7:56 PM
pkelsey added a comment to D8218: Close t_tfo_pending leaks.

I can only identify two circumstances where the t_tfo_pending counter on a listen socket will be incremented without a corresponding decrement ever occurring.

Oct 11 2016, 7:05 PM

Sep 1 2016

pkelsey committed rS305169: _taskqueue_start_threads() now fails if it doesn't actually start any threads..
_taskqueue_start_threads() now fails if it doesn't actually start any threads.
Sep 1 2016, 2:06 AM
pkelsey closed D7701: _taskqueue_start_threads() should only claim success if it starts a thread by committing rS305169: _taskqueue_start_threads() now fails if it doesn't actually start any threads..
Sep 1 2016, 2:06 AM

Aug 30 2016

pkelsey added a comment to D7701: _taskqueue_start_threads() should only claim success if it starts a thread.
In D7701#159878, @jhb wrote:

This looks correct though I'd be surprised if any code actually ran into it. (Or at least, if kthread_add() is failing your system is probably in trouble already.)

Aug 30 2016, 2:20 AM

Aug 29 2016

pkelsey retitled D7701: _taskqueue_start_threads() should only claim success if it starts a thread from to _taskqueue_start_threads() should only claim success if it starts a thread.
Aug 29 2016, 9:46 PM

Dec 28 2015

pkelsey committed rS292823: MFC r292706:.
MFC r292706:
Dec 28 2015, 2:43 AM

Dec 24 2015

pkelsey committed rS292706: Implementation of server-side TCP Fast Open (TFO) [RFC7413]..
Implementation of server-side TCP Fast Open (TFO) [RFC7413].
Dec 24 2015, 7:10 PM
pkelsey closed D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation by committing rS292706: Implementation of server-side TCP Fast Open (TFO) [RFC7413]..
Dec 24 2015, 7:10 PM

Dec 23 2015

pkelsey added a comment to D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.

Absent any issues raised, I am going to commit this sometime in the next 24-48 hours.

Dec 23 2015, 12:01 AM

Dec 17 2015

pkelsey updated D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 17 2015, 5:58 PM

Dec 9 2015

pkelsey updated the diff for D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.

Fixed issue with discarding TFO ACKs in tcp_do_segment() and fixed issue with snd_una not being properly advanced when TFO connections transition to ESTABLISHED, also in tcp_do_segment(). Also some minor cleanups (comments, whitespace, etc).

Dec 9 2015, 7:43 PM
pkelsey added inline comments to D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 9 2015, 7:31 PM

Dec 3 2015

pkelsey added inline comments to D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 3 2015, 2:31 AM
pkelsey added inline comments to D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 3 2015, 12:53 AM

Dec 2 2015

pkelsey updated the diff for D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.

File contents for tcp_fastopen.[ch] were missing from the initial diff.

Dec 2 2015, 9:13 PM
pkelsey added a reviewer for D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation: rrs.
Dec 2 2015, 8:06 PM
pkelsey added inline comments to D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 2 2015, 8:00 PM
pkelsey retitled D4350: TCP Fast Open (TFO) [RFC7413] Server-side Implementation from to TCP Fast Open (TFO) [RFC7413] Server-side Implementation.
Dec 2 2015, 7:44 PM

Sep 7 2015

pkelsey added a comment to D3567: ignore ICMP need frag with equal or larger MTU offer.

I don't think we should be taking any

Sep 7 2015, 5:32 PM

Sep 4 2015

pkelsey added a comment to D3567: ignore ICMP need frag with equal or larger MTU offer.

We shouldn't be taking any action at all based on a NEEDFRAG message that suggests (or from which we deduce) a new mtu that is the same or larger than the current one. I think the clearest expression of this idea is to only call tcp_mtudisc() from tcp_ctlinput() when the new mtu could decrease tp->t_maxseg (and remove the comment that tcp_mtudisc() 'does the right thing').

Sep 4 2015, 3:53 PM

Jul 29 2015

pkelsey committed rS286027: Revert r265338, r271089 and r271123 as those changes do not handle.
Revert r265338, r271089 and r271123 as those changes do not handle
Jul 29 2015, 5:59 PM

Jul 21 2015

pkelsey committed rS285762: MFC r285567:.
MFC r285567:
Jul 21 2015, 6:38 PM

Jul 20 2015

pkelsey committed rD47025: Remove trailing "@" from team approval example in Committer's Guide,.
Remove trailing "@" from team approval example in Committer's Guide,
Jul 20 2015, 6:28 PM
pkelsey closed D3128: Remove trailing "@" from team approval example in Committer's Guide by committing rD47025: Remove trailing "@" from team approval example in Committer's Guide,.
Jul 20 2015, 6:28 PM
pkelsey updated the diff for D3128: Remove trailing "@" from team approval example in Committer's Guide.

Moved </entry> to the end of the </screen> line in the last edit.

Jul 20 2015, 3:01 AM
pkelsey updated the diff for D3128: Remove trailing "@" from team approval example in Committer's Guide.

Adjusted markup for team approval example per wblock.

Jul 20 2015, 2:15 AM

Jul 19 2015

pkelsey retitled D3128: Remove trailing "@" from team approval example in Committer's Guide from to Remove trailing "@" from team approval example in Committer's Guide.
Jul 19 2015, 8:50 PM

Jul 18 2015

pkelsey committed rS285665: MFC r285275 (only the part that fixes PR 199568):.
MFC r285275 (only the part that fixes PR 199568):
Jul 18 2015, 1:29 AM

Jul 17 2015

pkelsey closed D3060: Don't use uninitialized timestamp echo reply value. by committing rS285657: Check TCP timestamp option flag so that the automatic receive buffer.
Jul 17 2015, 5:37 PM
pkelsey committed rS285657: Check TCP timestamp option flag so that the automatic receive buffer.
Check TCP timestamp option flag so that the automatic receive buffer
Jul 17 2015, 5:37 PM

Jul 15 2015

pkelsey committed rS285605: MFC r285190:.
MFC r285190:
Jul 15 2015, 4:57 PM
pkelsey committed rS285604: MFC r285188:.
MFC r285188:
Jul 15 2015, 4:56 PM
pkelsey committed rS285593: Revert inadvertent change to amd64/GENERIC..
Revert inadvertent change to amd64/GENERIC.
Jul 15 2015, 1:05 AM
pkelsey committed rS285592: Add netmap support for ixgbe SRIOV VFs (that is, to if_ixv)..
Add netmap support for ixgbe SRIOV VFs (that is, to if_ixv).
Jul 15 2015, 1:02 AM
pkelsey closed D2923: netmap support for ixgbe SRIOV VFs (if_ixv) by committing rS285592: Add netmap support for ixgbe SRIOV VFs (that is, to if_ixv)..
Jul 15 2015, 1:02 AM
pkelsey committed rS285590: Fix igxbe SRIOV VF (if_ixv) initialization bugs. The MAC address for.
Fix igxbe SRIOV VF (if_ixv) initialization bugs. The MAC address for
Jul 15 2015, 12:36 AM
pkelsey closed D2922: Fix ixgbe SRIOV bugs by committing rS285590: Fix igxbe SRIOV VF (if_ixv) initialization bugs. The MAC address for.
Jul 15 2015, 12:36 AM

Jul 13 2015

pkelsey added a comment to D2922: Fix ixgbe SRIOV bugs.

It's been two weeks with no further input. I just pinged rstone directly with a last-call. If there are no further objections in the next 8 hours or so, I plan to commit this as-is.

Jul 13 2015, 6:35 PM
pkelsey added a comment to D3060: Don't use uninitialized timestamp echo reply value..
In D3060#60525, @hiren wrote:

https://www.ietf.org/rfc/rfc1323.txt mentions:

The Timestamp Echo Reply field (TSecr) is only valid if the ACK
bit is set in the TCP header; if it is valid, it echos a times-
tamp value that was sent by the remote TCP in the TSval field
of a Timestamps option.  When TSecr is not valid, its value
must be zero.

Which I brought up to Patrick but then he mentioned that while following this and always initializing the tsecr field early in tcpopt processing would be the correct thing to do but the code is already established elsewhere to 1) check the option flag for the timestamp option and then 2) check if the value is not zero.

And I agree that because of that reasoning, the proposed fix seems sufficient to me.

Also adding network so someone else can also review this.

Jul 13 2015, 1:58 AM

Jul 12 2015

pkelsey retitled D3060: Don't use uninitialized timestamp echo reply value. from to Don't use uninitialized timestamp echo reply value..
Jul 12 2015, 7:00 AM

Jul 8 2015

pkelsey committed rS285299: MFV r285292:.
MFV r285292:
Jul 8 2015, 11:58 PM
pkelsey committed rS285292: Merge upstream fix to eliminate build-breaking gcc warnings of no.
Merge upstream fix to eliminate build-breaking gcc warnings of no
Jul 8 2015, 9:33 PM
pkelsey closed D3007: Merge tcpdump 4.7.4 to head by committing rS285275: MFV r285191: tcpdump 4.7.4..
Jul 8 2015, 4:19 PM
pkelsey committed rS285275: MFV r285191: tcpdump 4.7.4..
MFV r285191: tcpdump 4.7.4.
Jul 8 2015, 4:19 PM
pkelsey committed rS285273: Replace use of .Po Pc with the preferred .Pq for single line.
Replace use of .Po Pc with the preferred .Pq for single line
Jul 8 2015, 4:17 PM
pkelsey closed D3000: Replace use of .Po Pc with the preferred .Pq for single line enclosures in iovctl.conf(5), iovctl(8), pci(9), and pci_iov_schema(9). by committing rS285273: Replace use of .Po Pc with the preferred .Pq for single line.
Jul 8 2015, 4:17 PM

Jul 7 2015

pkelsey removed a reviewer for D3000: Replace use of .Po Pc with the preferred .Pq for single line enclosures in iovctl.conf(5), iovctl(8), pci(9), and pci_iov_schema(9).: rstone.
Jul 7 2015, 8:53 PM