Re: [RFC ipsec-next v2 4/8] iptfs: sysctl: allow configuration of global default values

[Sabrina Dubroca <sd@queasysnail.net>]

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