| Message ID | 20231113035219.920136-5-chopps@chopps.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add IP-TFS mode to xfrm | expand |
2023-11-12, 22:52:15 -0500, Christian Hopps wrote: > From: Christian Hopps <chopps@labn.net> > > Add sysctls for the changing the IPTFS default SA values. > > Signed-off-by: Christian Hopps <chopps@labn.net> > --- > Documentation/networking/xfrm_sysctl.rst | 29 ++++++++++++++++++ > include/net/netns/xfrm.h | 6 ++++ > include/net/xfrm.h | 7 +++++ > net/xfrm/xfrm_sysctl.c | 38 ++++++++++++++++++++++++ > 4 files changed, 80 insertions(+) > > diff --git a/Documentation/networking/xfrm_sysctl.rst b/Documentation/networking/xfrm_sysctl.rst > index 47b9bbdd0179..9e628806c110 100644 > --- a/Documentation/networking/xfrm_sysctl.rst > +++ b/Documentation/networking/xfrm_sysctl.rst > @@ -9,3 +9,32 @@ XFRM Syscall > > xfrm_acq_expires - INTEGER > default 30 - hard timeout in seconds for acquire requests > + > +xfrm_iptfs_maxqsize - UNSIGNED INTEGER > + The default IPTFS max output queue size in octets. The output queue is > + where received packets destined for output over an IPTFS tunnel are > + stored prior to being output in aggregated/fragmented form over the > + IPTFS tunnel. > + > + Default 1M. > + > +xfrm_iptfs_drptime - UNSIGNED INTEGER nit: Can we make those names a bit more human-readable? xfrm_iptfs_droptime, or possibly xfrm_iptfs_drop_time for better consistency with the netlink API. > + The default IPTFS drop time in microseconds. The drop time is the amount > + of time before a missing out-of-order IPTFS tunnel packet is considered > + lost. See also the reorder window. > + > + Default 1s (1000000). > + > +xfrm_iptfs_idelay - UNSIGNED INTEGER I would suggest xfrm_iptfs_initial_delay (or at least init_delay like the netlink attribute). > + The default IPTFS initial output delay in microseconds. The initial > + output delay is the amount of time prior to servicing the output queue > + after queueing the first packet on said queue. Does that also apply if the queue was fully drained (no traffic for a while) and starts being used again? That might be worth documenting either way (sorry, I haven't processed the main patch far enough to answer this question myself yet). And it might be worth mentioning that all these sysctls can be overridden per SA via the netlink API. > + Default 0. > + > +xfrm_iptfs_rewin - UNSIGNED INTEGER xfrm_iptfs_reorderwin (or reorder_win, or reorder_window)? I'd also make the equivalent netlink attribute's name a bit longer (at least spell out REORDER, I can live with WIN for WINDOW). [...] > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index c9bb0f892f55..d2e87344d175 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -2190,4 +2190,11 @@ static inline int register_xfrm_interface_bpf(void) > > #endif > > +#if IS_ENABLED(CONFIG_XFRM_IPTFS) > +#define XFRM_IPTFS_DEFAULT_MAX_QUEUE_SIZE (1024 * 1024) > +#define XFRM_IPTFS_DEFAULT_INIT_DELAY_USECS (0) > +#define XFRM_IPTFS_DEFAULT_DROP_TIME_USECS (1000000) > +#define XFRM_IPTFS_DEFAULT_REORDER_WINDOW (3) > +#endif nit: move those to net/xfrm/xfrm_sysctl.c ? they're only used in that file. > diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c > index 7fdeafc838a7..bf8e73a6c38e 100644 > --- a/net/xfrm/xfrm_sysctl.c > +++ b/net/xfrm/xfrm_sysctl.c [...] > @@ -55,6 +87,12 @@ int __net_init xfrm_sysctl_init(struct net *net) > table[1].data = &net->xfrm.sysctl_aevent_rseqth; > table[2].data = &net->xfrm.sysctl_larval_drop; > table[3].data = &net->xfrm.sysctl_acq_expires; > +#if IS_ENABLED(CONFIG_XFRM_IPTFS) > + table[4].data = &net->xfrm.sysctl_iptfs_drptime; > + table[5].data = &net->xfrm.sysctl_iptfs_idelay; > + table[6].data = &net->xfrm.sysctl_iptfs_maxqsize; > + table[7].data = &net->xfrm.sysctl_iptfs_rewin; > +#endif Is it time to switch to a loop like in ipv6_sysctl_net_init? See commit d2f7e56d1e40 ("ipv6: Use math to point per net sysctls into the appropriate struct net"). OTOH we don't add sysctls for xfrm very often so it's not critical.
Sabrina Dubroca <sd@queasysnail.net> writes: > 2023-11-12, 22:52:15 -0500, Christian Hopps wrote: >> From: Christian Hopps <chopps@labn.net> >> >> Add sysctls for the changing the IPTFS default SA values. >> >> Signed-off-by: Christian Hopps <chopps@labn.net> >> --- >> Documentation/networking/xfrm_sysctl.rst | 29 ++++++++++++++++++ >> include/net/netns/xfrm.h | 6 ++++ >> include/net/xfrm.h | 7 +++++ >> net/xfrm/xfrm_sysctl.c | 38 ++++++++++++++++++++++++ >> 4 files changed, 80 insertions(+) >> >> diff --git a/Documentation/networking/xfrm_sysctl.rst b/Documentation/networking/xfrm_sysctl.rst >> index 47b9bbdd0179..9e628806c110 100644 >> --- a/Documentation/networking/xfrm_sysctl.rst >> +++ b/Documentation/networking/xfrm_sysctl.rst >> @@ -9,3 +9,32 @@ XFRM Syscall >> >> xfrm_acq_expires - INTEGER >> default 30 - hard timeout in seconds for acquire requests >> + >> +xfrm_iptfs_maxqsize - UNSIGNED INTEGER >> + The default IPTFS max output queue size in octets. The output queue is >> + where received packets destined for output over an IPTFS tunnel are >> + stored prior to being output in aggregated/fragmented form over the >> + IPTFS tunnel. >> + >> + Default 1M. >> + >> +xfrm_iptfs_drptime - UNSIGNED INTEGER > > nit: Can we make those names a bit more human-readable? > xfrm_iptfs_droptime, or possibly xfrm_iptfs_drop_time for better > consistency with the netlink API. Changed to xfrm_iptfs_drop_time. >> + The default IPTFS drop time in microseconds. The drop time is the amount >> + of time before a missing out-of-order IPTFS tunnel packet is considered >> + lost. See also the reorder window. >> + >> + Default 1s (1000000). >> + >> +xfrm_iptfs_idelay - UNSIGNED INTEGER > > I would suggest xfrm_iptfs_initial_delay (or at least init_delay like > the netlink attribute). Changed to xfrm_iptfs_init_delay. > >> + The default IPTFS initial output delay in microseconds. The initial >> + output delay is the amount of time prior to servicing the output queue >> + after queueing the first packet on said queue. > > Does that also apply if the queue was fully drained (no traffic for a > while) and starts being used again? That might be worth documenting > either way (sorry, I haven't processed the main patch far enough to > answer this question myself yet). Added: "This applies anytime the output queue was previously empty." > And it might be worth mentioning that all these sysctls can be > overridden per SA via the netlink API. The description of these values as the "default"s implies the fact that they can be changed, I think. >> + Default 0. >> + >> +xfrm_iptfs_rewin - UNSIGNED INTEGER > > xfrm_iptfs_reorderwin (or reorder_win, or reorder_window)? > I'd also make the equivalent netlink attribute's name a bit longer (at > least spell out REORDER, I can live with WIN for WINDOW). > Changed to xfrm_iptfs_reorder_window. Also renamed the netlink attribute REORD_WIN to match (i.e., not XFRMA_IPTFS_REORDER_WINDOW). > [...] >> diff --git a/include/net/xfrm.h b/include/net/xfrm.h >> index c9bb0f892f55..d2e87344d175 100644 >> --- a/include/net/xfrm.h >> +++ b/include/net/xfrm.h >> @@ -2190,4 +2190,11 @@ static inline int register_xfrm_interface_bpf(void) >> >> #endif >> >> +#if IS_ENABLED(CONFIG_XFRM_IPTFS) >> +#define XFRM_IPTFS_DEFAULT_MAX_QUEUE_SIZE (1024 * 1024) >> +#define XFRM_IPTFS_DEFAULT_INIT_DELAY_USECS (0) >> +#define XFRM_IPTFS_DEFAULT_DROP_TIME_USECS (1000000) >> +#define XFRM_IPTFS_DEFAULT_REORDER_WINDOW (3) >> +#endif > > nit: move those to net/xfrm/xfrm_sysctl.c ? they're only used in that file. Rather than move them directly above where they are only used I just removed them entirely. Thanks, Chris. >> diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c >> index 7fdeafc838a7..bf8e73a6c38e 100644 >> --- a/net/xfrm/xfrm_sysctl.c >> +++ b/net/xfrm/xfrm_sysctl.c > [...] >> @@ -55,6 +87,12 @@ int __net_init xfrm_sysctl_init(struct net *net) >> table[1].data = &net->xfrm.sysctl_aevent_rseqth; >> table[2].data = &net->xfrm.sysctl_larval_drop; >> table[3].data = &net->xfrm.sysctl_acq_expires; >> +#if IS_ENABLED(CONFIG_XFRM_IPTFS) >> + table[4].data = &net->xfrm.sysctl_iptfs_drptime; >> + table[5].data = &net->xfrm.sysctl_iptfs_idelay; >> + table[6].data = &net->xfrm.sysctl_iptfs_maxqsize; >> + table[7].data = &net->xfrm.sysctl_iptfs_rewin; >> +#endif > > Is it time to switch to a loop like in ipv6_sysctl_net_init? See > commit d2f7e56d1e40 ("ipv6: Use math to point per net sysctls into the > appropriate struct net"). OTOH we don't add sysctls for xfrm very > often so it's not critical.
diff --git a/Documentation/networking/xfrm_sysctl.rst b/Documentation/networking/xfrm_sysctl.rst index 47b9bbdd0179..9e628806c110 100644 --- a/Documentation/networking/xfrm_sysctl.rst +++ b/Documentation/networking/xfrm_sysctl.rst @@ -9,3 +9,32 @@ XFRM Syscall xfrm_acq_expires - INTEGER default 30 - hard timeout in seconds for acquire requests + +xfrm_iptfs_maxqsize - UNSIGNED INTEGER + The default IPTFS max output queue size in octets. The output queue is + where received packets destined for output over an IPTFS tunnel are + stored prior to being output in aggregated/fragmented form over the + IPTFS tunnel. + + Default 1M. + +xfrm_iptfs_drptime - UNSIGNED INTEGER + The default IPTFS drop time in microseconds. The drop time is the amount + of time before a missing out-of-order IPTFS tunnel packet is considered + lost. See also the reorder window. + + Default 1s (1000000). + +xfrm_iptfs_idelay - UNSIGNED INTEGER + The default IPTFS initial output delay in microseconds. The initial + output delay is the amount of time prior to servicing the output queue + after queueing the first packet on said queue. + + Default 0. + +xfrm_iptfs_rewin - UNSIGNED INTEGER + The default IPTFS reorder window size. The reorder window size dictates + the maximum number of IPTFS tunnel packets in a sequence that may arrive + out of order. + + Default 3. diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h index bd7c3be4af5d..d5ad2155d0bb 100644 --- a/include/net/netns/xfrm.h +++ b/include/net/netns/xfrm.h @@ -65,6 +65,12 @@ struct netns_xfrm { u32 sysctl_aevent_rseqth; int sysctl_larval_drop; u32 sysctl_acq_expires; +#if IS_ENABLED(CONFIG_XFRM_IPTFS) + u32 sysctl_iptfs_drptime; + u32 sysctl_iptfs_idelay; + u32 sysctl_iptfs_maxqsize; + u32 sysctl_iptfs_rewin; +#endif u8 policy_default[XFRM_POLICY_MAX]; diff --git a/include/net/xfrm.h b/include/net/xfrm.h index c9bb0f892f55..d2e87344d175 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -2190,4 +2190,11 @@ static inline int register_xfrm_interface_bpf(void) #endif +#if IS_ENABLED(CONFIG_XFRM_IPTFS) +#define XFRM_IPTFS_DEFAULT_MAX_QUEUE_SIZE (1024 * 1024) +#define XFRM_IPTFS_DEFAULT_INIT_DELAY_USECS (0) +#define XFRM_IPTFS_DEFAULT_DROP_TIME_USECS (1000000) +#define XFRM_IPTFS_DEFAULT_REORDER_WINDOW (3) +#endif + #endif /* _NET_XFRM_H */ diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c index 7fdeafc838a7..bf8e73a6c38e 100644 --- a/net/xfrm/xfrm_sysctl.c +++ b/net/xfrm/xfrm_sysctl.c @@ -10,6 +10,12 @@ static void __net_init __xfrm_sysctl_init(struct net *net) net->xfrm.sysctl_aevent_rseqth = XFRM_AE_SEQT_SIZE; net->xfrm.sysctl_larval_drop = 1; net->xfrm.sysctl_acq_expires = 30; +#if IS_ENABLED(CONFIG_XFRM_IPTFS) + net->xfrm.sysctl_iptfs_maxqsize = XFRM_IPTFS_DEFAULT_MAX_QUEUE_SIZE; + net->xfrm.sysctl_iptfs_drptime = XFRM_IPTFS_DEFAULT_DROP_TIME_USECS; + net->xfrm.sysctl_iptfs_idelay = XFRM_IPTFS_DEFAULT_INIT_DELAY_USECS; + net->xfrm.sysctl_iptfs_rewin = XFRM_IPTFS_DEFAULT_REORDER_WINDOW; +#endif } #ifdef CONFIG_SYSCTL @@ -38,6 +44,32 @@ static struct ctl_table xfrm_table[] = { .mode = 0644, .proc_handler = proc_dointvec }, +#if IS_ENABLED(CONFIG_XFRM_IPTFS) + { + .procname = "xfrm_iptfs_drptime", + .maxlen = sizeof(uint), + .mode = 0644, + .proc_handler = proc_douintvec + }, + { + .procname = "xfrm_iptfs_idelay", + .maxlen = sizeof(uint), + .mode = 0644, + .proc_handler = proc_douintvec + }, + { + .procname = "xfrm_iptfs_maxqsize", + .maxlen = sizeof(uint), + .mode = 0644, + .proc_handler = proc_douintvec + }, + { + .procname = "xfrm_iptfs_rewin", + .maxlen = sizeof(uint), + .mode = 0644, + .proc_handler = proc_douintvec + }, +#endif {} }; @@ -55,6 +87,12 @@ int __net_init xfrm_sysctl_init(struct net *net) table[1].data = &net->xfrm.sysctl_aevent_rseqth; table[2].data = &net->xfrm.sysctl_larval_drop; table[3].data = &net->xfrm.sysctl_acq_expires; +#if IS_ENABLED(CONFIG_XFRM_IPTFS) + table[4].data = &net->xfrm.sysctl_iptfs_drptime; + table[5].data = &net->xfrm.sysctl_iptfs_idelay; + table[6].data = &net->xfrm.sysctl_iptfs_maxqsize; + table[7].data = &net->xfrm.sysctl_iptfs_rewin; +#endif /* Don't export sysctls to unprivileged users */ if (net->user_ns != &init_user_ns) {