[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <m2h6is6zko.fsf@ja.int.chopps.org>
Date: Thu, 01 Feb 2024 08:57:25 -0500
From: Christian Hopps <chopps@...n.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Christian Hopps <chopps@...pps.org>, devel@...ux-ipsec.org, Steffen
Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org, Christian
Hopps <chopps@...n.net>
Subject: Re: [RFC ipsec-next v2 4/8] iptfs: sysctl: allow configuration of
global default values
Sabrina Dubroca <sd@...asysnail.net> writes:
> 2023-11-12, 22:52:15 -0500, Christian Hopps wrote:
>> From: Christian Hopps <chopps@...n.net>
>>
>> Add sysctls for the changing the IPTFS default SA values.
>>
>> Signed-off-by: Christian Hopps <chopps@...n.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.
Download attachment "signature.asc" of type "application/pgp-signature" (859 bytes)
Powered by blists - more mailing lists