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

[Christian Hopps <chopps@labn.net>]

[-- Attachment #1: Type: text/plain, Size: 4750 bytes --]


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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 858 bytes --]