pkelsey (Patrick Kelsey)
User

Projects

User Details

User Since
Jun 2 2014, 3:47 PM (211 w, 5 d)

Recent Activity

Mar 7 2018

pkelsey added a comment to D14048: Modify handling of TCP_RFC7413 (TFO) kernel option.

Going to commit this soon as the review has been open for nearly one month and has been idle for two weeks.

Mar 7 2018, 4:47 PM
pkelsey added inline comments to D14554: Support printing of the TCP fast open client side cache.
Mar 7 2018, 4:43 PM
pkelsey closed D14048: Modify handling of TCP_RFC7413 (TFO) kernel option.
Mar 7 2018, 4:42 PM
pkelsey closed D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Mar 7 2018, 4:42 PM
pkelsey updated the diff for D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).

Addressed reviewer comments.

Mar 7 2018, 4:42 PM
pkelsey added a comment to D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).

Going to commit this soon as there has not been any further review comments in nearly three weeks.

Mar 7 2018, 4:42 PM

Mar 6 2018

pkelsey accepted D14554: Support printing of the TCP fast open client side cache.

Looks good. Thanks.

Mar 6 2018, 3:46 AM

Feb 26 2018

pkelsey committed rS330035: Ensure signed comparison to avoid false trip of assert during VNET teardown..
Ensure signed comparison to avoid false trip of assert during VNET teardown.
Feb 26 2018, 8:31 PM
pkelsey committed rS330002: Greatly reduce the number of #ifdefs supporting the TCP_RFC7413 kernel option..
Greatly reduce the number of #ifdefs supporting the TCP_RFC7413 kernel option.
Feb 26 2018, 3:04 AM
pkelsey committed rS330001: This is an implementation of the client side of TCP Fast Open (TFO).
This is an implementation of the client side of TCP Fast Open (TFO)
Feb 26 2018, 2:53 AM
pkelsey committed rS330000: Fix harmless locking bug in tfp_fastopen_check_cookie()..
Fix harmless locking bug in tfp_fastopen_check_cookie().
Feb 26 2018, 2:43 AM
pkelsey closed D14046: Fix harmless locking bug in tcp_fastopen_check_cookie().
Feb 26 2018, 2:43 AM

Jan 31 2018

pkelsey added inline comments to D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Jan 31 2018, 3:39 AM

Jan 30 2018

pkelsey added inline comments to D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Jan 30 2018, 6:45 AM

Jan 28 2018

pkelsey added inline comments to D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Jan 28 2018, 3:35 PM

Jan 25 2018

pkelsey committed rS328392: MFC of r305169:.
MFC of r305169:
Jan 25 2018, 7:27 AM
pkelsey committed rS328391: MFC of r316630, r316631, r316632, r316633, r316634.
MFC of r316630, r316631, r316632, r316633, r316634
Jan 25 2018, 7:19 AM
pkelsey created D14048: Modify handling of TCP_RFC7413 (TFO) kernel option.
Jan 25 2018, 7:10 AM
pkelsey added a dependent revision for D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two): D14048: Modify handling of TCP_RFC7413 (TFO) kernel option.
Jan 25 2018, 7:09 AM
pkelsey created D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Jan 25 2018, 6:18 AM
pkelsey added a dependent revision for D14046: Fix harmless locking bug in tcp_fastopen_check_cookie(): D14047: TCP Fast Open (TFO) [RFC7413] Client-side Implementation (take two).
Jan 25 2018, 6:18 AM
pkelsey created D14046: Fix harmless locking bug in tcp_fastopen_check_cookie().
Jan 25 2018, 6:12 AM
pkelsey abandoned D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.

Breaking into separate reviews.

Jan 25 2018, 6:01 AM
pkelsey committed rD51393: Partially document __FreeBSD_version value 1101507..
Partially document __FreeBSD_version value 1101507.
Jan 25 2018, 2:58 AM
pkelsey committed rS328386: MFC r316648:.
MFC r316648:
Jan 25 2018, 2:45 AM

Jan 4 2018

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

Jan 2 2018

pkelsey added inline comments to D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.
Jan 2 2018, 9:44 PM
pkelsey added reviewers for D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation: hiren, tuexen.
Jan 2 2018, 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. :-)

Jan 2 2018, 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
Jan 2 2018, 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.

Jan 2 2018, 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.

Jan 2 2018, 5:47 PM

Dec 31 2017

pkelsey created D13718: TCP Fast Open (TFO) [RFC7413] Client-side Implementation.
Dec 31 2017, 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