Differential D4055
First step to modularize TCP including two added tcp modules that can be played with. Authored by rrs on Oct 31 2015, 11:39 PM. Tags None Referenced Files
Details
As previously discussed on the transport lists and call, here is a first cut Most all other functions are accounted for however (enough that we can try some different TCP functionality without Pound on it at NF as I develop several missing RFC's and other fun such at NF.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions all the rest I will fix shortly
Comment Actions Randall, Thanks a lot for this work. We need this so badly :-) It looks good to me. There are a few tiny comments inline and a couple general comments:
I am so excited to have this finally in our stack!
Comment Actions Adds and updates comments. I did find a small difference below
Comment Actions In another review, Han's commented on having source code in modules. Since Comment Actions Ok after reviewing a bit of the tcp_function_set routine, I updated it to
Comment Actions Hi Randall, I have a few specific comments in-line, and two other questions:
Also, I had a thought about testing this. In particular, rwatson@ had indicated that he wondered how it would impact branch prediction, etc. If we can find a machine with a replicable TCP testing load, we should be able to use PMC to count various statistics (including the # of branch prediction misses, etc.) both with and without this change. I don't know if that will completely satisfy rwatson's concerns, but it will help quantify the present impact of this change. Again, I really appreciate your work on this. When we're talking about style matters, you know everything else must be pretty good. ;-) Jonathan
Comment Actions This updates for the latest comments and adds back the missing Comment Actions
Comment Actions I still wonder why we need a new user-space program, when you could use sysctls to do the same thing.
I still wonder about the convention for naming structure members (see previous comment; e.g. tfs_pcbcnt vs. pcbcnt).
Comment Actions Yes, but there is nothing that the user-space program does that could not be done through a sysctl. You could trivially modify the net.inet.tcp.functions_available sysctl to also return the number of pcb's. In fact, I have offered to modify it to return the exact output the user-space program currently produces. At that point, we could do away with the program and the net.inet.tcp.functions_list_detail sysctl. I am not opposed to a user-space program when it adds value. However, there is a cost to adding additional code that must be maintained. Here, the code doesn't seem to add value over what the sysctls provide (or could provide, with trivial changes). You talk of it allowing us to do more in the future. Once we are ready to do things that require a user-space program, we can add one then. There is no reason to rush to add one at this time. If we add one now, it will require maintenance and we will need to consider backwards compatibility when we try to add new features to the program. If you still think we really need a user-space program, I would ask that you at least get a second opinion on it. I could be wrong. Getting additional input may help clarify the correct direction.
Just a quick proposal: struct tcp_function_block {
char tfb_name[TCP_FUNCTION_NAME_LEN_MAX];
int (*tfb_tcp_output)(struct tcpcb *);
void (*tfb_tcp_do_segment)(struct mbuf *, struct tcphdr *,
struct socket *, struct tcpcb *,
int, int, uint8_t,
int);
int (*tfb_tcp_ctloutput)(struct socket *so, struct sockopt *sopt,
struct inpcb *inp, struct tcpcb *tp);
/* Optional memory allocation/free routine */
void (*tfb_init)(struct tcpcb *);
void (*tfb_fini)(struct tcpcb *);
/* Optional timers, must define all if you define one */
void (*tfb_tcp_timer_stop_all)(struct tcpcb *);
int (*tfb_tcp_timers_left)(struct tcpcb *);
void (*tfb_tcp_timer_activate)(struct tcpcb *,
uint32_t, u_int);
int (*tfb_tcp_timer_active)(struct tcpcb *, uint32_t);
void (*tfb_tcp_timer_stop)(struct tcpcb *, uint32_t);
volatile uint32_t tfb_refcnt;
uint32_t tfb_flags;
};
struct tcp_function {
TAILQ_ENTRY(tcp_function) tf_next;
struct tcp_function_block *tf_fb;
};
Comment Actions Jonathan I can be convinced *not* to have a user space program (though that is exactly what unix was founded upon, a Send me the code you think would make the sysctl work and you can convince me.. assuming the output
Comment Actions This updates the latest round from Jonathan. Note that the tfb_timer_stopall was changed to an int return. With the Comment Actions I think the infrastructure to manage TCP stacks is acceptable. There are still some style nits I pointed out (primarily, inconsistent use of '(ptr)' vs. '(ptr != NULL)', and the reverse of that). I also did not review the kernel module at all (and, rrs@ said he didn't expect that, since it was just an example). I think it should probably be committed to some "examples" or "sample" directory until someone reviews it and the community "adopts" it as something we want to support. With those caveats, I think the infrastructure to manage TCP stacks is acceptable. I think we all owe Randall some thanks, especially for working through so many comments. :-) Comment Actions
A huge 'Thank You' to both of you! | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||